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

Adds content to toolbar examples #8906

Merged
merged 5 commits into from
May 3, 2023
Merged

Conversation

edonehoo
Copy link
Contributor

@edonehoo edonehoo commented Apr 3, 2023

Makes progress on patternfly/patternfly-org#2990

@edonehoo edonehoo self-assigned this Apr 3, 2023
@patternfly-build
Copy link
Contributor

patternfly-build commented Apr 3, 2023

@nicolethoen
Copy link
Contributor

@edonehoo
There is an accessibility test failure because each of our toolbar examples was assigned an id (not quite sure why), and every heading on the page is also given an id. The Toolbar items heading you added is now conflicting with the id on toolbar in the Toolbar items example.
If you update the id on this line to something like 'toolbar-items-example' then the violation should be resolved.

Copy link
Member

@mcarrano mcarrano left a comment

Choose a reason for hiding this comment

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

It feels odd to me that Sticky Toolbar is the first example. It was not previously. Is there are reason we changed the order?

Also, @nicolethoen , I assume it's unrelated to this PR, but do we understand why we are see a vertical mis-alignment now of certain items in toolbar examples?

### Adjusting item spacers
### Toolbar item spacers

You may adjust the space between toolbar items to arrange them into groups. [Read our spacers documentation](/v4/guidelines/spacers) to learn more about using spacers.
Copy link
Contributor

Choose a reason for hiding this comment

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

This url leads to a 404 page. @mcarrano does/will this page exist in the updated page structure of PatternFly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the link for now to avoid the 404, but we can add it back when we figure out where to accurately point it

@nicolethoen
Copy link
Contributor

nicolethoen commented Apr 25, 2023

Also, @nicolethoen , I assume it's unrelated to this PR, but do we understand why we are see a vertical mis-alignment now of certain items in toolbar examples?

which examples? I am not seeing misalignment when I preview the toolbar examples @mcarrano

@mcarrano
Copy link
Member

@nicolethoen I see the problem here, for example: https://patternfly-react-pr-8906.surge.sh/components/toolbar#component-managed-toggle-groups

Screenshot 2023-04-25 at 2 08 26 PM

I only see this in Firefox. Not in Chrome.

@nicolethoen
Copy link
Contributor

Ah I am not seeing that on chrome - but i see it on firefox. bummer. We can open a follow up issue for that

@nicolethoen
Copy link
Contributor

@mcarrano here is the follow up issue to address select alignment in toolbars in firefox: patternfly/patternfly#5532

Copy link
Member

@mcarrano mcarrano left a comment

Choose a reason for hiding this comment

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

@nicolethoen Thanks for opening the follow-up. I approve then.

@edonehoo
Copy link
Contributor Author

edonehoo commented May 3, 2023

Regarding "It feels odd to me that Sticky Toolbar is the first example. It was not previously. Is there are reason we changed the order?": I thought that it seemed like the most "basic" example since it referred to the toolbar behavior itself, rather than the toolbar items. But, if it seems odd, that was my only reason. I went ahead and moved it back to its original location assuming that makes better sense overall!

@mcarrano
Copy link
Member

mcarrano commented May 3, 2023

Looks great. Thanks for the update @edonehoo .

@nicolethoen nicolethoen merged commit 2264d8a into patternfly:v5 May 3, 2023
10 checks passed
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.

None yet

5 participants