feat(JumpLinks): consumed Penta updates#10027
Conversation
| isActive?: boolean; | ||
| /** Href for this link */ | ||
| href?: string; | ||
| href: string; |
There was a problem hiding this comment.
This needs to be required, as without it the anchor internal to the component cannot receive keyboard focus.
If the intent is that a consumer can pass either an href to link to an ID on the page, or the onClick to handle this functionality themselves, then we would need to render either an anchor with an href or a button with the onClick, not an anchor with either/both.
There was a problem hiding this comment.
Should we make a codemod to at least warn consumers about this?
There was a problem hiding this comment.
| <span className={styles.jumpLinksLinkText}>{children}</span> | ||
| </a> | ||
| <span className={styles.jumpLinksLink}> | ||
| <a className={css(buttonStyles.button, buttonStyles.modifiers.link)} href={href} onClick={onClick}> |
There was a problem hiding this comment.
Passed the classes in instead of using Button with props just to retain the onClick event type. No strong opinion about this, so I can just update the onClick event type and use the Button component proper here instead if we want.
There was a problem hiding this comment.
I would update this to use the Button if possible.
|
Preview: https://patternfly-react-pr-10027.surge.sh A11y report: https://patternfly-react-pr-10027-a11y.surge.sh |
thatblindgeye
requested review from
a team,
andrew-ronaldson,
lboehling,
mfrances17,
nicolethoen and
srambach
and removed request for
a team
nicolethoen
left a comment
nicolethoen
left a comment
There was a problem hiding this comment.
It'd be nice to do a v6 org bump once this merges, then the jumplinks used navigate between examples in the docs can have the same hover styles in the workspace.
andrew-ronaldson
left a comment
andrew-ronaldson
left a comment
There was a problem hiding this comment.
sweet moves. Done like dinner.
| <span className={styles.jumpLinksLinkText}>{children}</span> | ||
| </a> | ||
| <span className={styles.jumpLinksLink}> | ||
| <a className={css(buttonStyles.button, buttonStyles.modifiers.link)} href={href} onClick={onClick}> |
There was a problem hiding this comment.
I would update this to use the Button if possible.
thatblindgeye
force-pushed
the
jumpLinksPenta
branch
from
0e5b35b to
4a94da0
Compare
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
7 participants
What: Closes #9994
Additional issues: