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): dropdown closes on click outside of menu area #2235

Merged
merged 5 commits into from Jun 13, 2019

Conversation

@jenny-s51
Copy link
Contributor

jenny-s51 commented Jun 12, 2019

What: closes #1562

@jenny-s51 jenny-s51 requested review from nicolethoen and redallen Jun 12, 2019
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Jun 12, 2019

Copy link
Contributor

karelhala left a comment

Looking good!

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jun 12, 2019

Codecov Report

Merging #2235 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2235      +/-   ##
==========================================
+ Coverage   79.88%   79.88%   +<.01%     
==========================================
  Files         668      668              
  Lines        8520     8521       +1     
  Branches      731      733       +2     
==========================================
+ Hits         6806     6807       +1     
  Misses       1362     1362              
  Partials      352      352
Flag Coverage Δ
#patternfly3 85.23% <ø> (ø) ⬆️
#patternfly4 74.89% <100%> (ø) ⬆️
#patternflymisc 95.79% <ø> (ø) ⬆️
Impacted Files Coverage Δ
...src/components/Pagination/PaginationOptionsMenu.js 92.85% <100%> (+0.54%) ⬆️
...ct-core/src/components/Pagination/OptionsToggle.js 80% <100%> (-3.34%) ⬇️
...ernfly-4/react-core/src/components/Alert/Alert.tsx 88.46% <0%> (+0.46%) ⬆️

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 c15f7e7...007a82e. Read the comment docs.

@jenny-s51 jenny-s51 requested a review from karelhala Jun 12, 2019
Copy link
Contributor

nicolethoen left a comment

LGTM! :)

@nicolethoen nicolethoen merged commit a2942c2 into patternfly:master Jun 13, 2019
2 checks passed
2 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tlabaj

This comment has been minimized.

Copy link
Contributor

tlabaj commented Jun 13, 2019

@mcoker this has been merged. But can you just take a look at it to make sure everything looks good. Thanks!

@nicolethoen

This comment has been minimized.

Copy link
Contributor

nicolethoen commented Jun 13, 2019

@mcoker @tlabaj I can address any concerns about this PR in PR #2256

@mcoker

This comment has been minimized.

Copy link
Contributor

mcoker commented Jun 13, 2019

@tlabaj @nicolethoen from reading jenn's notes in #1562, are we going to make this default functionality in the options menu? Looks like for this PR, the functionality was only added to the pagination component's options menu.

LGTM overall, but I did notice that clicking in the options menu text doesn't trigger the menu to close. Should it? I would expect it to if I'm clicking anywhere outside of the menu area.

Here's a screenshot with the text I'm referring to highlighted.

Screen Shot 2019-06-13 at 1 27 53 PM

@nicolethoen

This comment has been minimized.

Copy link
Contributor

nicolethoen commented Jun 13, 2019

@mcoker I just pushed a change to #2256 to make the options menu text click trigger a close. There is a build issue (I don't think related to my changes) so I'm not sure you can see the change in the preview yet.

@jenny-s51 jenny-s51 deleted the jenny-s51:iss1562_copy branch Jul 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.