Skip to content
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

feat(NavExpandable): allows title prop to accept ReactNodes #9264

Conversation

dvail
Copy link
Contributor

@dvail dvail commented Jun 12, 2023

Potential fix for patternfly/patternfly-design#1267

Changes the <NavExpandable> title prop to accept any ReactNode. This also adds an example to the documentation. (Is this wanted, or is it more of a distraction in it's current location?)

@dvail dvail changed the title Allows NavExpandable to accept ReactNode as a title prop feat(NavExpandable): allows title prop to accept ReactNodes Jun 12, 2023
@patternfly-build
Copy link
Contributor

patternfly-build commented Jun 12, 2023

@andrew-ronaldson
Copy link
Collaborator

Not sure where this fits into the component (core/react) but I noticed the nav text is centered and the label breaks to a new line. Can this be inline with the rest of the nav item title?
Screenshot 2023-08-16 at 12 21 24 PM

@kmcfaul
Copy link
Contributor

kmcfaul commented Aug 23, 2023

I think the line break is a result of using Flex, it will automatically collapse separate items into a stack when the window narrows. Wrapping both the span and Label in the example with a FlexItem will group the two slightly better (the label is still on a new line but matches the center alignment of the text). Is that close to what you're thinking or is the intent having the label to the right of the text?

Copy link
Contributor

@mcoker mcoker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@mcoker
Copy link
Contributor

mcoker commented Aug 24, 2023

After chatting with @tlabaj about it, it's probably better if we create a better HTML structure for markup (as opposed to just text) to go in the nav link. I think the main issue with the structure now would because the nav link is a flex layout, its children are going to be flex children, so passing something like "nav text " will not display that way since "nav text" and "" will be separate flex children. Both of them will be stretched, and the whitespace between them will not be preserved. It would be better if we added a wrapper for the nav link text/content that is not a flex layout, and that will be what holds the nav link text/content.

To do this in a way that isn't breaking and doesn't impact current users passing strings as the nav text, we could do this as an opt-in by changing the markup if anything other than a string is passed via something like:

  • Check the type of the value passed as the title prop
  • If the type is string, leave the markup as it is
  • Else, wrap the value of the title prop in a .pf-v5-c-nav__link-text element

Created a core issue for the initial work to add that element - patternfly/patternfly#5857

Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we incorporate @mcoker suggestion of checking type and rendering accordingly?

@mcoker
Copy link
Contributor

mcoker commented Aug 29, 2023

@tlabaj the core change was merged, though it's worth noting that there are no associated styles for the .pf-v5-c-nav__link-text element, so its className will probably need to be hardcoded in the react component - patternfly/patternfly#5868

Copy link
Collaborator

@andrew-ronaldson andrew-ronaldson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good to me. Thank you!

@tlabaj
Copy link
Contributor

tlabaj commented Oct 11, 2023

I could not push to this PR, so I created a new one to add the nav link text wrapper
#9740

@tlabaj
Copy link
Contributor

tlabaj commented Oct 26, 2023

Closing since this was resolved with PR #9740

@tlabaj tlabaj closed this Oct 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants