-
Notifications
You must be signed in to change notification settings - Fork 45
bugfix/COR-1069-inconsistent-link-hover-styles #4507
bugfix/COR-1069-inconsistent-link-hover-styles #4507
Conversation
…nts to be more generic and prevent faulty hover states;
<IconWrapper> | ||
{words[words.length - 1]} | ||
<IconSmall icon={icon} width={11} height={10} /> | ||
</IconWrapper> |
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.
Perhaps someone could elaborate on this functionality and why it was written like this initially? I do not understand why we would
- split all words only to correctly place the icon after the last word?
- adjust positioning of the icon based on whether there is only a single word or not?
- only use this logic when the
iconPlacement
evaluates toright
, and not alsoleft
?
I removed all this logic as I personally did not see any differences between the before and after versions of this component, and I feel my new solution is more straightforward and simple.
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 cannot think of why this would have been done this way in the past 🤷
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.
Correct me if im wrong, but from your 'After' screenshot, I understand that the entire link should not be underlined when hovering? 🤔
<IconWrapper> | ||
{words[words.length - 1]} | ||
<IconSmall icon={icon} width={11} height={10} /> | ||
</IconWrapper> |
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 cannot think of why this would have been done this way in the past 🤷
Good question! As discussed, I followed the requirements mentioned in the older COR-937 ticket. To reiterate: this link is not part of a paragraph and should therefore not contain an underline in either its normal or hover states, nor is it a link to a specific single article and should therefore not be bold. |
Summary
Notes
These changes are based upon the design proposals and work done as part of COR-937, for which the earlier pull request can be found here.
I clicked through the entire dashboard looking for any instances of
LinkWithIcon
andHeadingLinkWithIcon
to be sure that the simplified changes did not introduce any issues and meet the requirements specified.Screenshots
Both screenshots show a hover state, but no cursor.
Before
After