-
Notifications
You must be signed in to change notification settings - Fork 312
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
fix(icon): Allow icon to be focusable when not clickable #2217
fix(icon): Allow icon to be focusable when not clickable #2217
Conversation
…not-clickable-6965412634' into allow-icon-to-be-focusable-when-not-clickable-6965412634
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.
amazing work! had some unblocking comments
const iconLabel = useMemo(() => { | ||
if (type === AttentionBoxType.DANGER) { | ||
return "alert"; | ||
} | ||
|
||
if (type === AttentionBoxType.SUCCESS) { | ||
return "success"; | ||
} | ||
|
||
return "attention"; | ||
}, [type]); |
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 said those are redundant anyway, right?
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.
Yes, the icon is for decoration purpose only
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.
That's awesome!!
// TODO remove in next major | ||
// eslint-disable-next-line @typescript-eslint/no-unused-vars |
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.
maybe also add this in the type declaration as you did in other places, as it is easy to remove it from here and forget to remove from declaration. so even only there is enough instead of here, as this way it would show an error, but not the other way around
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.
Changed it
<Tooltip | ||
content={infoContent} | ||
referenceWrapperClassName={styles.infoTooltip} | ||
addKeyboardHideShowTriggersByDefault |
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.
btw, is this has a task to become default true at next major? just wondering 👀
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 didn't see one, so added it
@@ -66,6 +66,7 @@ export interface TextFieldProps extends VibeComponentProps { | |||
searchResultsContainerId?: string; | |||
activeDescendant?: string; | |||
/** Icon names labels for a11y */ | |||
/// TODO Remove layout in next major |
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.
💪🏼
@@ -69,7 +69,7 @@ export const States: Story = { | |||
<TextField placeholder="With icon" iconName={Email} size={TextField.sizes.MEDIUM} /> | |||
<TextField | |||
placeholder="With clickable icon" | |||
iconName={Email} | |||
iconName={Duplicate} |
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.
makes more sense for the sake of the example, good job 👏🏼
https://monday.monday.com/boards/3532714909/views/80492480/pulses/6965412634