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

feat(Dropdown): add split button action variant #3307

Merged
merged 9 commits into from Nov 21, 2019

Conversation

@kmcfaul
Copy link
Contributor

kmcfaul commented Nov 15, 2019

What: Closes #3226

Additional issues:

@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Nov 15, 2019

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

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 15, 2019

Codecov Report

Merging #3307 into master will increase coverage by <.01%.
The diff coverage is 65.51%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3307      +/-   ##
==========================================
+ Coverage   67.34%   67.34%   +<.01%     
==========================================
  Files         893      895       +2     
  Lines       24921    24958      +37     
  Branches     2146     2153       +7     
==========================================
+ Hits        16783    16809      +26     
- Misses       7133     7141       +8     
- Partials     1005     1008       +3
Flag Coverage Δ
#misc 95.45% <ø> (ø) ⬆️
#patternfly3 69.28% <ø> (ø) ⬆️
#patternfly4 64.6% <65.51%> (+0.01%) ⬆️
Impacted Files Coverage Δ
...eact-core/src/components/Dropdown/DropdownItem.tsx 68.42% <0%> (-3.81%) ⬇️
...ct-core/src/components/Dropdown/DropdownToggle.tsx 82.85% <0%> (-5.03%) ⬇️
...e/src/components/Dropdown/DropdownToggleAction.tsx 100% <100%> (ø)
...nfly-4/react-core/src/components/Dropdown/index.ts 100% <100%> (ø) ⬆️
...-core/src/components/Dropdown/DropdownItemIcon.tsx 50% <50%> (ø)
...e/src/components/Dropdown/InternalDropdownItem.tsx 72.36% <60%> (-1.25%) ⬇️
.../patternfly-react/src/components/Dropdown/index.js 90.76% <0%> (-1.96%) ⬇️

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 f5d61b2...c841aeb. Read the comment docs.

@tlabaj tlabaj requested a review from mcoker Nov 18, 2019
@tlabaj tlabaj added the ux review label Nov 18, 2019
@tlabaj tlabaj requested review from mcarrano and redallen Nov 18, 2019
Copy link
Contributor

mcoker left a comment

Hey @kmcfaul looks great! Left a small comment about the aria-label on a normal button with text in it.

This may be beyond the scope of this PR, but looks like the non-action split button checkboxes are using the <Checkbox> component and they should just be a normal <input type="checkbox">.

Should I open another issue for that?

Also I need to make some changes in core to support the expanded state. Currently there is a 1px grey border showing up over the 2px blue border.

Screen Shot 2019-11-18 at 12 33 34 PM

@kmcfaul

This comment has been minimized.

Copy link
Contributor Author

kmcfaul commented Nov 18, 2019

@mcoker Yeah if you can open up a bug that would be great. I've removed the aria-label.

Copy link
Member

mcarrano left a comment

This looks great @kmcfaul . While reviewing this it occurred to me that we should have included icons in front of each menu item for the second example. This will require a core update that I've discussed with @mcoker and opened a core issue here: patternfly/patternfly-next#2450

@tlabaj Can we hold on merging this for the moment to see if we can get this into the current release? If not, I would be OK to accept this and return to update later.

@mcoker

This comment has been minimized.

Copy link
Contributor

mcoker commented Nov 18, 2019

@kmcfaul the expanded border has been fixed in core patternfly/patternfly-next#2449 and is in 2.41.1

@mcoker

This comment has been minimized.

Copy link
Contributor

mcoker commented Nov 19, 2019

@kmcfaul patternfly/patternfly-next#2451 was merged in core to support adding icons to the dropdown component menu.

  • Adds a modifier to menu items (.pf-m-icon) to use on items that have an icon in them.
  • Adds a new element to the dropdown (.pf-c-dropdown__menu-item-icon) that will hold the icon/image/etc
@kmcfaul kmcfaul force-pushed the kmcfaul:split-action branch from 898b46c to 6271c9a Nov 19, 2019
Copy link
Contributor

mcoker left a comment

Nice! Thanks @kmcfaul!

Copy link
Contributor

tlabaj left a comment

Can you update the demo app and add some integration test please

Copy link
Contributor

mcoker left a comment

Hey @kmcfaul noticed a couple of other things.

Copy link
Member

mcarrano left a comment

Look great @kmcfaul !

@mcarrano mcarrano added ux approved and removed ux review labels Nov 20, 2019
@kmcfaul kmcfaul dismissed stale reviews from mcarrano and mcoker via 68cdb6a Nov 20, 2019
Copy link
Contributor

mcoker left a comment

This looks good. My comments about removing the ID shouldn't hold up merging this, but would be nice to see at some point 🤠

Copy link
Contributor

tlabaj left a comment

LGTM

@kmcfaul kmcfaul dismissed stale reviews from tlabaj, mcoker, and redallen via c841aeb Nov 20, 2019
@mcoker
mcoker approved these changes Nov 20, 2019
@tlabaj
tlabaj approved these changes Nov 21, 2019
Copy link
Contributor

tlabaj left a comment

LGTM

@tlabaj tlabaj merged commit 625ea59 into patternfly:master Nov 21, 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
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Nov 21, 2019

Your changes have been released in:

  • @patternfly/react-catalog-view-extension@1.1.35
  • @patternfly/react-core@3.123.0
  • @patternfly/react-docs@4.16.41
  • @patternfly/react-inline-edit-extension@2.13.6
  • demo-app-ts@3.12.0
  • @patternfly/react-integration@3.12.0
  • @patternfly/react-table@2.24.38
  • @patternfly/react-topology@2.11.24
  • @patternfly/react-virtualized-extension@1.3.37

Thanks for your contribution! 🎉

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.