fix(Pagination): Styling discrepancy with HTML version by Venefilyn · Pull Request #2904 · patternfly/patternfly-react · GitHub

What: Fix differences with upstream. This makes Pagination be identical to HTML variant

Additional issues: Fixes #2903

Note, work in progress. Trying to figure out the best approach for fixing the font size and padding of the dropdown button as well as the font color. Fixed

PatternFly-React preview: https://patternfly-react-pr-2904.surge.sh

Trying to change DropdownToggle in OptionsToggle to OptionsMenuToggleWithText doesn't help much. HTML variant has button.pf-c-options-menu__toggle-button, but I'm unable to see how we can get this without the .pf-c-dropdown__toggle being added

@spytec @tlabaj I took a quick look at this. Looks like the behavior and form are now correct but font size and spacing are still off as noted.

Had a chat on Slack with mnolting in #patternfly-core. The culprit is .pf-c-dropdown__toggle being applied, which is due to using the DropdownToggle component in OptionsMenu. What I can see this will need to have a slightly larger refactor to drop DropdownToggle and still have the same functionality

I've managed to sync everything with HTML version. One note to keep in mind, to get around applying .pf-c-dropdown__toggle class to the dropdown I added a DropdownContext.Provider with toggleClass value of a space which is shown in the outputted HTML.

How it looks:

Will rebase with master once build process is fixed

Looks like .pf-c-options-menu is nested in another .pf-c-options-menu, and in the top level .pf-c-options-menu, there is a #pagination-options-menu-top-label element with "Items per
page:" that we don't have in core.

"Items per page" is an aria-label on the toggle button in core, and the .pf-c-options-menu__menu element's aria-labelledby is set to the ID of the toggle button.

Also looks like .pf-c-options-menu__menu-item is listed twice on the menu items

And for the .pf-c-options-menu__menu-item-icon, can we just apply that class directly to the <svg> and remove the ?

Fixed all but the .pf-c-options-menu__menu-item-icon. Putting on the SVG itself didn't work. Personally I think it would be better to keep it to an as it is in core so that the elements don't change

I also added export * from './DropdownWithContext'; in Dropdown/index.tsx so it can be imported in PaginationOptionsMenu.tsx

@spytec thanks! I opened patternfly/patternfly#2297 re: the SVG/icon class.

This looks good to me, although I'm still seeing this element in the react component that isn't in the core examples:

And in the react component, .pf-c-options-menu__toggle-button has aria-label="Select" and in core, we have aria-label="Items per page".

@jessiehuff or @jgiardino can you confirm if we need to make updates there?

Glanced over that, sorry! I can fix it soon-ish if need be

Replying to @mcoker's comment above, there is a related open issue to the aria label applied to the toggle: #1882

Ignore this, see next comment
However, when I compare the html in core to the html in react, they are different, and the solution we want is dependent on which html we're using. So that issue might need to be updated depending on that.

The main difference I'm seeing is that in core, the text that displays in the toggle "x-y of z items" is not part of the button. But in react, it is.

Doh! I was looking at the current version of the react component, and not the fixes included in this PR. 😜

This hidden span was an artifact of the old way we were labelling the toggle. It can be removed.
Items per page:

That issue I mention above is still relevant, but not a blocker for this PR.

@mcoker @spytec As far as the aria-label, Select is just a default in the OptionsToggle class, we can edit to match core with Items per page. For the top level label, I believe that's something added for the accessibility of the example (@jgiardino). Edit: answered above

Changes look good and will make styling of the compact pagination uniform with core as well.

I think this looks good from a UX perspective. There are still some spacing differences with core, but after discussing with @mcoker it looks like this is an artifact of the different ways icons are rendered between core and React. It becomes very pronounced here with the horizontal stacking of icons in the design. @LHinson something to note for future improvement, but beyond the scope of this PR

Fixed the last bit with Select aria-label so it's now Items per page. Also removed old labelling Items per page: as suggested

LGTM

Great job @spytec, thanks for making these updates!!

@spytec We can merge this one once the conflicts are resolved

Codecov Report

❗ No coverage uploaded for pull request base (master@5a539b3). Click here to learn what that means.
The diff coverage is 85.71%.

@@            Coverage Diff            @@
##             master    #2904   +/-   ##
=========================================
  Coverage          ?   68.94%           
=========================================
  Files             ?      852           
  Lines             ?    23290           
  Branches          ?     1792           
=========================================
  Hits              ?    16057           
  Misses            ?     6344           
  Partials          ?      889

Flag	Coverage Δ
#misc	95.45% <ø> (?)
#patternfly3	69.16% <ø> (?)
#patternfly4	68% <85.71%> (?)

Impacted Files	Coverage Δ
...-core/src/components/Pagination/ToggleTemplate.tsx	42.85% <ø> (ø)
...eact-core/src/components/Pagination/Pagination.tsx	82.53% <ø> (ø)
...eact-core/src/components/Pagination/Navigation.tsx	88.09% <100%> (ø)
...rc/components/Pagination/PaginationOptionsMenu.tsx	93.54% <100%> (ø)
...nfly-4/react-core/src/components/Dropdown/index.ts	100% <100%> (ø)
...t-core/src/components/Pagination/OptionsToggle.tsx	44% <50%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a539b3...ebff13a. Read the comment docs.

LGTM

Venefilyn changed the title fix(Pagination): Discrepancy with HTML version fix(Pagination): Styling discrepancy with HTML version Sep 11, 2019

tlabaj added the css review label Sep 12, 2019

tlabaj requested review from mcarrano and mcoker September 12, 2019 19:19

tlabaj added the ux review label Sep 12, 2019

tlabaj requested a review from jschuler September 12, 2019 19:19

Venefilyn marked this pull request as ready for review September 13, 2019 08:38

tlabaj assigned kmcfaul Sep 17, 2019

Venefilyn force-pushed the fix/pagination-button branch from a708338 to 4ac125a Compare September 23, 2019 12:21

kmcfaul previously approved these changes Sep 24, 2019

View reviewed changes

mcarrano previously approved these changes Sep 24, 2019

View reviewed changes

mcarrano added ux approved and removed ux review labels Sep 24, 2019

kmcfaul mentioned this pull request Sep 24, 2019

feat(Pagination): add compact variation flag #2998

Merged

rachael-phillips added this to the 2019.08 milestone Sep 24, 2019

Venefilyn dismissed stale reviews from mcarrano and kmcfaul via 6729d62 September 25, 2019 11:48

kmcfaul previously approved these changes Sep 25, 2019

View reviewed changes

tlabaj previously approved these changes Sep 25, 2019

View reviewed changes

mcoker previously approved these changes Sep 25, 2019

View reviewed changes

jgiardino mentioned this pull request Sep 25, 2019

Pagination - add prop for page size menu toggle label #1882

Closed

tlabaj reviewed Sep 25, 2019

View reviewed changes

dlabrecq previously approved these changes Sep 25, 2019

View reviewed changes

Venefilyn added 3 commits September 26, 2019 11:17

Rebase with upstream

e640ff5

fix(Pagination): Updated aria-label and removed old labelling

4a2fc0e

test(Pagination): Fix snapshot

ebff13a

Venefilyn dismissed stale reviews from dlabrecq, mcoker, tlabaj, and kmcfaul via ebff13a September 26, 2019 09:18

Venefilyn force-pushed the fix/pagination-button branch from 0dbd9e4 to ebff13a Compare September 26, 2019 09:18

kmcfaul approved these changes Sep 26, 2019

View reviewed changes

mcoker approved these changes Sep 26, 2019

View reviewed changes

tlabaj approved these changes Sep 26, 2019

View reviewed changes

tlabaj merged commit 7e6e586 into patternfly:master Sep 26, 2019

Venefilyn deleted the fix/pagination-button branch September 26, 2019 14:45

fix(Pagination): Styling discrepancy with HTML version by Venefilyn · Pull Request #2904 · patternfly/patternfly-react · GitHub

Uh oh!

fix(Pagination): Styling discrepancy with HTML version - #2904

fix(Pagination): Styling discrepancy with HTML version#2904tlabaj merged 3 commits intopatternfly:masterpatternfly/patternfly-react:masterfrom Venefilyn:fix/pagination-buttonVenefilyn/patternfly-react:fix/pagination-buttonCopy head branch name to clipboard

Conversation

Venefilyn commented Sep 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patternfly-build commented Sep 11, 2019

Uh oh!

Venefilyn commented Sep 11, 2019

Uh oh!

mcarrano commented Sep 12, 2019

Uh oh!

Venefilyn commented Sep 12, 2019

Uh oh!

Venefilyn commented Sep 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcoker commented Sep 17, 2019

Uh oh!

Venefilyn commented Sep 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcoker commented Sep 23, 2019

Uh oh!

Venefilyn commented Sep 24, 2019

Uh oh!

jgiardino commented Sep 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jgiardino commented Sep 24, 2019

Uh oh!

kmcfaul left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

Venefilyn commented Sep 25, 2019

Uh oh!

tlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

mcoker left a comment

Choose a reason for hiding this comment

Uh oh!

tlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Sep 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

fix(Pagination): Styling discrepancy with HTML version#2904
tlabaj merged 3 commits into
patternfly:masterfrom
Venefilyn:fix/pagination-button

Venefilyn commented Sep 11, 2019 •
edited
Loading

Venefilyn commented Sep 13, 2019 •
edited
Loading

Venefilyn commented Sep 23, 2019 •
edited
Loading

jgiardino commented Sep 24, 2019 •
edited
Loading

kmcfaul left a comment •
edited
Loading

codecov-io commented Sep 26, 2019 •
edited
Loading