feat: Created PropsTable component structure#37
feat: Created PropsTable component structure#37wise-king-sullyman merged 4 commits intopatternfly:mainfrom
Conversation
| it('Does not render component description by default', () => { | ||
| render(<PropsTable componentName={componentName} />) | ||
|
|
||
| expect(screen.queryByTestId('component-description')).not.toBeInTheDocument() |
There was a problem hiding this comment.
Feel like this happens every once in a blue moon, but only reason I'm doing this is because I feel like checking the specific description string doesn't exist could potentially lead to false passes. This at least checks the description wrapper (and its content) doesn't exist at all.
src/components/PropsTable.tsx
Outdated
| const betaDeprecatedProps = hasPropsToRender | ||
| ? publicProps.filter((prop) => prop.isBeta && prop.isDeprecated) | ||
| : [] | ||
|
|
||
| if (betaDeprecatedProps.length) { | ||
| // eslint-disable-next-line no-console | ||
| console.error( | ||
| `The following ${componentName} props have both the isBeta and isDeprecated tag: ${betaDeprecatedProps.map((prop) => prop.name).join(', ')}`, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Think this could be done in the renderTagLabel function? Seems like that might be cleaner.
There was a problem hiding this comment.
Yeah we could, only reason I originally did it this way was to capture all props that fit the criteria at once rather than one at a time, but doing it in the renderTagLabel would negate the need for some of this code so I'm for it
src/components/PropsTable.tsx
Outdated
| {prop.isRequired ? ( | ||
| <> | ||
| <span | ||
| aria-hidden="true" | ||
| className={css(textStyles.textColorRequired)} | ||
| > | ||
| * | ||
| </span> | ||
| <span | ||
| className={css(accessibleStyles.screenReader)} | ||
| > | ||
| required | ||
| </span> | ||
| </> | ||
| ) : ( | ||
| '' | ||
| )}{' '} |
There was a problem hiding this comment.
What would you think about moving this into its own function/component so that we can have a little bit less logic in the JSX?
| />, | ||
| ) | ||
|
|
||
| expect(screen.getByText(componentDescription)).toBeVisible() |
There was a problem hiding this comment.
Nit: if you're using the testId to check that the description isn't rendering in the last test I would expect an assertion that it is rendering in this test.
Up to you whether you have both assertions or assert against the text content of the queried test id.
| consoleSpy.mockRestore() | ||
| }) | ||
|
|
||
| it('Does not throw error if isBeta and isDeprecated both are not passed', () => { |
There was a problem hiding this comment.
Nit for readability:
| it('Does not throw error if isBeta and isDeprecated both are not passed', () => { | |
| it('Does not throw error if neither isBeta nor isDeprecated are passed', () => { |
src/pages/index.astro
Outdated
| componentDescription="The container to render a list of alerts." | ||
| /> | ||
| <PropsTable componentName="AlertTitle" /> | ||
| </MainLayout> |
There was a problem hiding this comment.
Did you intend to keep these changes in the PR?
There was a problem hiding this comment.
Just for now, to show what the various renders look like in case anyone had thoughts/concerns or anything (also to get design input on the Label choices, though we can do that another time if need be; I can otherwise back that file change out in my next push if things look good to you)
|
@wise-king-sullyman spoke to Lucia and she gave feedback on the labels so I updated those accordingly (gray fill for deprecated, blue fill for beta). Other feedback should be resolved. |
|
🎉 This PR is included in version 1.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
2 participants
Towards #15
Note that I updated the index file to render some dummy data, I'll back that out after this gets reviews in case anyone has comments on the visuals.
Things to consider: