feat(labels): add deprecated/demo/beta labels to headers + nav items #3547
Conversation
|
PF4 preview: https://patternfly-org-pr-3547-v4.surge.sh/v4 |
mcarrano
left a comment
mcarrano
left a comment
There was a problem hiding this comment.
@nicolethoen this looks great! Just two small things to address...
- The design calls for an 8px spacing between the nav names and the labels. Looks like you have more than that.
- I did a search for examples that are currently labeled with the word Legacy. There are two: Navigation - Legacy tertiary (https://www.patternfly.org/v4/components/navigation#legacy-tertiary) and Legacy Spinner (https://www.patternfly.org/v4/components/spinner/html#basic-legacy and https://www.patternfly.org/v4/components/spinner/html#sizes-legacy) Can we mark these examples as deprecated and remove the word "legacy?"
lboehling
left a comment
lboehling
left a comment
There was a problem hiding this comment.
thank you @nicolethoen! I also noted the two changes @mcarrano just mentioned above, but other than that this looks great!
I've made a note to update these when I make the corresponding changes in core/react as follow up as soon as this merges. |
mcarrano
left a comment
mcarrano
left a comment
There was a problem hiding this comment.
Looks great. Thanks @nicolethoen !
| </Tooltip> | ||
| )} | ||
| {isDeprecated && ( | ||
| <Tooltip content="Deprecated components are available for use but are no longer being maintained or enhanced."> |
There was a problem hiding this comment.
Maybe a Surge issue not updating from a previous commit, but I'm seeing slightly different tooltip messages for these three items (Beta, Demo, Deprecated)
There was a problem hiding this comment.
good catch! i'll fix that
There was a problem hiding this comment.
Sorry - the text in my screenshot above actually maps to the tooltip in mdx.js, but curious if this text should match or maybe live in a single variable somewhere that can be referenced?
There was a problem hiding this comment.
I made them match. We don't right now have like a constants or strings file. Maybe that's something else we can clean up to add when we clean up the docs-framework in the future.
| You can find the <Link to={newImplementationLink}>updated implementation here</Link>. | ||
| </React.Fragment> | ||
| )} | ||
| To learn more about the process, visit our <Link to="/get-started/about#major-release-cadence">about page</Link>. |
There was a problem hiding this comment.
FYI there's currently no space between sentences
| }) => { | ||
| const isDeprecated = sources.some(source => source.source === "react-deprecated") && !sources.some(source => source.source === "react"); | ||
| const isBeta = sources.some(source => source.beta) | ||
| const isDemo = sources.some(source => source.source === "react-demos" || source.source === "html-demos") && !sources.some(source => source.source === "react" || source.source === "html"); |
There was a problem hiding this comment.
Something we're guilty of throughout docs-framework are these PF-specific hard coded values. Now that the docs-framework is a package used outside just the PF website build we should try to allow these to be customized by the user whenever possible.
There was a problem hiding this comment.
FYI this is widespread throughout the package, this works for now but noting for possible future cleanup effort.
There was a problem hiding this comment.
yeah and i'm not sure if I can programmatically determine that a deprecated label can be added to the Example title without some sort of data being manually stored in the component-data. That was going to be my fall back, but this felt like less maintenance for now.
| {(deprecated || source === 'react-deprecated') && ( | ||
| <InlineAlert title="Deprecated feature" variant="warning"> | ||
| This implementation has been deprecated in favor of a newer implementation, and is no longer getting maintained or enhanced. | ||
| {newImplementationLink && ( |
There was a problem hiding this comment.
We're not parsing newImplementationLink from the markdown anywhere currently so there's no way to trigger this code. We'd need to add
newImplementationLink: frontmatter.newImplementationLink || false
where we're defining pageData in mdx.js (similar to your added code for deprecated, beta, and demo).
|
@nicolethoen a few callouts, mostly nits that could be resolved in follow-up issues if needed other than the |
nicolethoen
dismissed stale reviews from lboehling, mcarrano, and wise-king-sullyman
via
b677a4a
6 participants
Closes #3370
Closes #3371
Closes #3467
Closes #3489
Implements designs from design issue patternfly/patternfly-design#1239
Note: I need to add some frontmatter metadata to a number of core/react to make banners appear at the top of some deprecated tabs. This PR is meant to enable the displaying of labels/banners, and still requires some follow up work in other repos.