-
Notifications
You must be signed in to change notification settings - Fork 99
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[USH-1610] On edit survey screen, show the user the type of field #1465
Conversation
Ready for review @Angamanga
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a tiny comment for removing a comment. I think for making the tooltip accessible, we could add a new ticket for that.
(helperField) => field.input === helperField.input && field.type === helperField.type, | ||
)[0]; | ||
//--- Product decision: Change Label if it's a "Select" field----------------- | ||
const originalLabel = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this comment belongs in a ticket, not in the codebase :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About the comment, the reason for adding it is based on the discussion I had with Tim. The word "select" is still used in other places e.g. the Add field modal popup when adding fields to the survey. But we are only changing the "select" on the hover text.
At the moment Tim, says he'd have to discuss with other team about if it should also change in other places. So whether it eventually changes or not, the comment is just to help remember or point attention to that slight change - A ticket would usually get closed and forgotten (happened many times) @Angamanga
---------------------------------------------*/ | ||
this.fieldHover = event.type === 'mouseenter'; | ||
this.fieldId = field.id; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works, with the mouse. Is there a way to make this available also for keyboard-users? 😬 I think perhaps with focus and blur-event + aria-labels? 🤔
tabindex="-1" | ||
matTooltipPosition="right" | ||
matTooltip="{{ field.label_original }}" | ||
> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't handle this in other places so it might be time for a separate ticket for making the tooltip accessible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is this part too... 👍🏼
class="field-item__main-inner" | ||
(mouseenter)="trackHoverEvent($event, field)" | ||
(mouseleave)="trackHoverEvent($event, field)" | ||
> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to think about the keyboard-users here as well, not just the hovering?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This crossed my mind too. One reason why I added the videos about the 5 different options to the ticket, so that people can check it out, see and make comments @Angamanga
The most accessible looking option is the one where all the icons show up already without having to hover. Just that there's the problem of having too much or much more elements/information on the UI.
I've tried one or 2 things accessibility wise on this hover option we are going with, non is successful so far.
I will try again, and let you know how it goes:
This works, with the mouse. Is there a way to make this available also for keyboard-users? 😬 I think perhaps with focus and blur-event + aria-labels? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you Mary! 🙌
615d3a0
to
bd33e69
Compare
bd33e69
to
ba03d4f
Compare
No description provided.