Skip to content

docs(Button): combined aria-disabled tooltip examples into single example#7911

Merged
tlabaj merged 6 commits intopatternfly:mainfrom
andyyvo:iss7620
Sep 12, 2022
Merged

docs(Button): combined aria-disabled tooltip examples into single example#7911
tlabaj merged 6 commits intopatternfly:mainfrom
andyyvo:iss7620

Conversation

@andyyvo
Copy link
Copy Markdown
Contributor

@andyyvo andyyvo commented Aug 31, 2022

What: Closes #7620 . Just cleaned up the two tooltip examples into one.

Additional issues:

@patternfly-build
Copy link
Copy Markdown
Collaborator

patternfly-build commented Aug 31, 2022

Comment thread packages/react-core/src/components/Button/examples/Button.md
Copy link
Copy Markdown
Contributor

@nicolethoen nicolethoen left a comment

Choose a reason for hiding this comment

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

This looks good after you delete the old example file.

Although, I have a question for @mcarrano and/or @edonehoo - do you want us to put all disabled + aria-disabled examples together in the button docs?
And I know you didn't update the example titles, but now that there is only one 'aria-disabled' example, you can remove the word 'examples' from the title on line 93

@tlabaj tlabaj requested a review from mcarrano September 1, 2022 15:42
@mcarrano
Copy link
Copy Markdown
Member

mcarrano commented Sep 1, 2022

do you want us to put all disabled + aria-disabled examples together in the button docs?

Not sure. @edonehoo what do you think?

@nicolethoen
Copy link
Copy Markdown
Contributor

Im meaning like location-wise all the examples grouped in the same general area on the page. Not combined into one example

@andyyvo
Copy link
Copy Markdown
Contributor Author

andyyvo commented Sep 8, 2022

@nicolethoen @mcarrano @edonehoo any updates on this?

@edonehoo
Copy link
Copy Markdown
Contributor

edonehoo commented Sep 8, 2022

Thanks for the poke! I think it makes sense to move them near the other disabled example and update the header.

@andyyvo
Copy link
Copy Markdown
Contributor Author

andyyvo commented Sep 8, 2022

@nicolethoen @tlabaj how did you say I could go about getting this integration test to work?

@nicolethoen
Copy link
Copy Markdown
Contributor

@andyyvo try rebasing and see if it passes the next time

@tlabaj
Copy link
Copy Markdown
Contributor

tlabaj commented Sep 9, 2022

@nicolethoen @tlabaj how did you say I could go about getting this integration test to work?

If you click on Details for the failed job, you should be able to restart it.

@evwilkin
Copy link
Copy Markdown
Member

evwilkin commented Sep 9, 2022

@andyyvo this is showing a merge conflict, but once that's resolved LGTM

Copy link
Copy Markdown
Contributor

@kmcfaul kmcfaul left a comment

Choose a reason for hiding this comment

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

Looks good just needs a rebase

Copy link
Copy Markdown
Contributor

@thatblindgeye thatblindgeye left a comment

Choose a reason for hiding this comment

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

Other than previous comments and merge conflicts, looks good!

export const ButtonAriaDisabled: React.FunctionComponent = () => (
<React.Fragment>
<Button isAriaDisabled>Primary aria disabled</Button> <Button isAriaDisabled>Secondary aria disabled</Button>{' '}
<Button isAriaDisabled>Primary aria disabled</Button>{' '}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Once this lint error is resolved we can take it in.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For some reason, the lint error is simply the new {' '} I added to give spacing between the examples. If I get rid of it, there are no issues. But it makes two of the buttons right next to each other

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

hmmm I am not sure why it is complaining about this line in particular since we have {' '} elsewhere.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it is ok to remove it for now. Some of our other examples have the buttons right next to each other.

@tlabaj tlabaj merged commit 8dffbaf into patternfly:main Sep 12, 2022
@patternfly-build
Copy link
Copy Markdown
Collaborator

Your changes have been released in:

  • eslint-plugin-patternfly-react@4.76.7
  • @patternfly/react-catalog-view-extension@4.88.7
  • @patternfly/react-charts@6.90.7
  • @patternfly/react-code-editor@4.78.7
  • @patternfly/react-console@4.88.7
  • @patternfly/react-core@4.237.7
  • @patternfly/react-docs@5.98.7
  • @patternfly/react-icons@4.88.7
  • @patternfly/react-inline-edit-extension@4.82.7
  • demo-app-ts@4.197.7
  • @patternfly/react-integration@4.199.7
  • @patternfly/react-log-viewer@4.82.7
  • @patternfly/react-styles@4.87.7
  • @patternfly/react-table@4.106.7
  • @patternfly/react-tokens@4.89.7
  • @patternfly/react-topology@4.84.7
  • @patternfly/react-virtualized-extension@4.84.7
  • transformer-cjs-imports@4.75.7

Thanks for your contribution! 🎉

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.

Button - combine aria-disabled tooltip examples into single example

10 participants