FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

feat(Dropdown): add split button action variant by kmcfaul · Pull Request #3307 · patternfly/patternfly-react · GitHub

/ patternfly-react Public

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

Merged
tlabaj merged 9 commits into
patternfly:masterfrom
kmcfaul:split-action
Nov 21, 2019
ConversationCommits (9)ChecksFiles changed
Merged

feat(Dropdown): add split button action variant#3307
tlabaj merged 9 commits into
patternfly:masterfrom
kmcfaul:split-action

Conversation

kmcfaul commented Nov 15, 2019

Copy link
Copy Markdown
Contributor

What: Closes #3226

Additional issues:

patternfly-build commented Nov 15, 2019

Copy link
Copy Markdown
Collaborator

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

codecov-io commented Nov 15, 2019 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

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

@@            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 requested a review from mcoker November 18, 2019 15:20
tlabaj requested review from mcarrano and redallen November 18, 2019 15:24
mcoker reviewed Nov 18, 2019
View reviewed changes

mcoker left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

kmcfaul commented Nov 18, 2019

Copy link
Copy Markdown
Contributor Author

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

mcarrano mentioned this pull request Nov 18, 2019
Closed
mcarrano reviewed Nov 18, 2019
View reviewed changes

mcarrano left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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#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 commented Nov 18, 2019

Copy link
Copy Markdown
Contributor

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

mcoker commented Nov 19, 2019

Copy link
Copy Markdown
Contributor

@kmcfaul patternfly/patternfly#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

mcoker previously approved these changes Nov 19, 2019
View reviewed changes

mcoker left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Nice! Thanks @kmcfaul!

tlabaj suggested changes Nov 19, 2019
View reviewed changes

tlabaj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

mcoker reviewed Nov 19, 2019
View reviewed changes

mcoker left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Hey @kmcfaul noticed a couple of other things.

Comment thread packages/patternfly-4/react-core/src/components/Dropdown/DropdownToggleAction.tsx Outdated
Comment thread packages/patternfly-4/react-core/src/components/Dropdown/examples/Dropdown.md Outdated
mcarrano previously approved these changes Nov 20, 2019
View reviewed changes

mcarrano left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Look great @kmcfaul !

kmcfaul dismissed stale reviews from mcarrano and mcoker via 68cdb6a November 20, 2019 16:06
redallen previously approved these changes Nov 20, 2019
View reviewed changes
mcoker previously approved these changes Nov 20, 2019
View reviewed changes

mcoker left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

tlabaj previously approved these changes Nov 20, 2019
View reviewed changes

tlabaj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

kmcfaul dismissed stale reviews from tlabaj, mcoker, and redallen via c841aeb November 20, 2019 21:48
tlabaj approved these changes Nov 21, 2019
View reviewed changes

tlabaj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

tlabaj merged commit 625ea59 into patternfly:master Nov 21, 2019

patternfly-build commented Nov 21, 2019

Copy link
Copy Markdown
Collaborator

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! 🎉

gdoyle1 mentioned this pull request Nov 25, 2019
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcoker mcoker approved these changes
+3 more reviewers
tlabaj tlabaj approved these changes
redallen redallen approved these changes
mcarrano mcarrano left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

redallen

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Split button variation

7 participants

Back | FazBrowse Home | New Git URL