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

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

Merged
merged 3 commits into from Sep 26, 2019

Conversation

@SpyTec
Copy link
Contributor

SpyTec commented Sep 11, 2019

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

image

@SpyTec SpyTec changed the title fix(Pagination): Discrepancy with HTML version fix(Pagination): Styling discrepancy with HTML version Sep 11, 2019
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Sep 11, 2019

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

@SpyTec

This comment has been minimized.

Copy link
Contributor Author

SpyTec commented Sep 11, 2019

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

@tlabaj tlabaj added the css review label Sep 12, 2019
@tlabaj tlabaj requested review from mcoker and mcarrano Sep 12, 2019
@tlabaj tlabaj added the ux review label Sep 12, 2019
@tlabaj tlabaj requested a review from jschuler Sep 12, 2019
@mcarrano

This comment has been minimized.

Copy link
Member

mcarrano commented Sep 12, 2019

@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.

@SpyTec

This comment has been minimized.

Copy link
Contributor Author

SpyTec commented Sep 12, 2019

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

@SpyTec SpyTec marked this pull request as ready for review Sep 13, 2019
@SpyTec

This comment has been minimized.

Copy link
Contributor Author

SpyTec commented Sep 13, 2019

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.

image

How it looks:
image

Will rebase with master once build process is fixed

@mcoker

This comment has been minimized.

Copy link
Contributor

mcoker commented Sep 17, 2019

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.

Screen Shot 2019-09-17 at 1 58 29 PM

"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

Screen Shot 2019-09-17 at 2 00 44 PM

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

Screen Shot 2019-09-17 at 4 11 23 PM

@SpyTec SpyTec force-pushed the SpyTec:fix/pagination-button branch from a708338 to 4ac125a Sep 23, 2019
@SpyTec

This comment has been minimized.

Copy link
Contributor Author

SpyTec commented Sep 23, 2019

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 <i> 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

@mcoker

This comment has been minimized.

Copy link
Contributor

mcoker commented Sep 23, 2019

@SpyTec thanks! I opened patternfly/patternfly-next#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:

Screen Shot 2019-09-23 at 4 20 36 PM

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?

@SpyTec

This comment has been minimized.

Copy link
Contributor Author

SpyTec commented Sep 24, 2019

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

@jgiardino

This comment has been minimized.

Copy link
Collaborator

jgiardino commented Sep 24, 2019

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.

@jgiardino

This comment has been minimized.

Copy link
Collaborator

jgiardino commented Sep 24, 2019

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.
<span id="pagination-options-menu-top-label" hidden="">Items per page:</span>

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

Copy link
Contributor

kmcfaul left a comment

@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.

Copy link
Member

mcarrano left a comment

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

@mcarrano mcarrano added ux approved and removed ux review labels Sep 24, 2019
@rachael-phillips rachael-phillips added this to the 2019.08 milestone Sep 24, 2019
@SpyTec SpyTec dismissed stale reviews from mcarrano and kmcfaul via 6729d62 Sep 25, 2019
@SpyTec

This comment has been minimized.

Copy link
Contributor Author

SpyTec commented Sep 25, 2019

Fixed the last bit with Select aria-label so it's now Items per page. Also removed old labelling <span id="pagination-options-menu-top-label" hidden="">Items per page:</span> as suggested

Copy link
Contributor

tlabaj left a comment

LGTM

Copy link
Contributor

mcoker left a comment

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

Copy link
Contributor

tlabaj left a comment

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

@SpyTec SpyTec dismissed stale reviews from dlabrecq, mcoker, tlabaj, and kmcfaul via ebff13a Sep 26, 2019
@SpyTec SpyTec force-pushed the SpyTec:fix/pagination-button branch from 0dbd9e4 to ebff13a Sep 26, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Sep 26, 2019

Codecov Report

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

Impacted file tree graph

@@            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.

@mcoker
mcoker approved these changes Sep 26, 2019
@tlabaj
tlabaj approved these changes Sep 26, 2019
Copy link
Contributor

tlabaj left a comment

LGTM

@tlabaj tlabaj merged commit 7e6e586 into patternfly:master Sep 26, 2019
8 checks passed
8 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_integration Your tests passed on CircleCI!
Details
ci/circleci: build_pf3_docs Your tests passed on CircleCI!
Details
ci/circleci: build_pf4_docs Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test_jest_other Your tests passed on CircleCI!
Details
ci/circleci: test_jest_pf4 Your tests passed on CircleCI!
Details
ci/circleci: upload_docs Your tests passed on CircleCI!
Details
@SpyTec SpyTec deleted the SpyTec:fix/pagination-button branch Sep 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.