Skip to content

fix(DataList): Add dropdown and button actions to DataList #1079

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.

Sign up for GitHub

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

Jump to bottom
Merged
dlabaj merged 1 commit into from
Mar 14, 2019
Merged

fix(DataList): Add dropdown and button actions to DataList #1079

dlabaj merged 1 commit into from
Mar 14, 2019

Conversation

@boaz0
Copy link
Member

@boaz0 boaz0 commented Dec 18, 2018

This is a follow-up to #927
Now you can add Dropdown or Button to DataListAction.
Please feel free to review the API, I did it as simple as I could but if you believe it needs another abstraction or logical layer, I would like to hear.

cc @jgiardino @jschuler @christiemolloy

@patternfly-build
Copy link
Collaborator

patternfly-build commented Dec 18, 2018

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

@coveralls
Copy link

coveralls commented Dec 18, 2018 •
edited
Loading

Pull Request Test Coverage Report for Build 3879

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 81.203%
Totals Coverage Status
Change from base Build 3866: 0.0%
Covered Lines: 4367
Relevant Lines: 5074
💛 - Coveralls

@tlabaj tlabaj added the PF4 label Dec 18, 2018
dlabaj
dlabaj previously approved these changes Jan 3, 2019
View reviewed changes
tlabaj
tlabaj previously approved these changes Jan 9, 2019
View reviewed changes
@tlabaj
Copy link
Contributor

tlabaj commented Jan 9, 2019

@boaz0 Please resolve conflicts.

@boaz0 boaz0 dismissed stale reviews from tlabaj and dlabaj via dfaff7d January 9, 2019 21:49
@boaz0 boaz0 changed the title [WIP] fix(DataList): Add dropdown and button actions to DataList fix(DataList): Add dropdown and button actions to DataList Jan 9, 2019
@boaz0
Copy link
Member Author

boaz0 commented Jan 9, 2019

@tlabaj rebased.

@tlabaj tlabaj requested a review from mcoker January 11, 2019 16:58
@tlabaj tlabaj requested a review from mcarrano January 11, 2019 17:01
mcarrano
mcarrano reviewed Jan 11, 2019
View reviewed changes
Copy link
Member

@mcarrano mcarrano left a comment

Choose a reason for hiding this comment

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

Looks fine to me, although I don't see an example with a dropdown. Are we talking about having a kabob?

@mcoker
Copy link
Contributor

mcoker commented Jan 11, 2019

Looks good to me, but should the dropdown at the end of a row default to use the align-right example as seen on https://pf-next.com/components/dropdown/examples/?

karelhala
karelhala suggested changes Jan 20, 2019
View reviewed changes
Copy link
Contributor

@karelhala karelhala left a comment

Choose a reason for hiding this comment

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

I don't think that passing button with kebab icon without any children is behavior described by core pattern.

@tlabaj tlabaj requested a review from christiemolloy January 28, 2019 15:24
christiemolloy
christiemolloy suggested changes Jan 28, 2019
View reviewed changes
Copy link
Member

@christiemolloy christiemolloy left a comment

Choose a reason for hiding this comment

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

  • In some of the examples, the kebabs aren't clickable, and are a different color from Core.
  • In the example where there is a Delete button, what is the purpose of "Delete" should it actually remove the line? Right now, it isn't functioning.

@tlabaj tlabaj self-assigned this Mar 6, 2019
@tlabaj
Copy link
Contributor

tlabaj commented Mar 8, 2019

@boaz0 what is the status of this PR? Looks like there are some outstanding comments from @christiemolloy that should be address?

@boaz0
fix(DataList): Add dropdown and button actions to DataList
56e3b82 
Signed-off-by: Boaz Shuster <boaz.shuster.github@gmail.com>
@codecov-io
Copy link

codecov-io commented Mar 14, 2019 •
edited
Loading

Codecov Report

Merging #1079 into master will decrease coverage by 0.01%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1079      +/-   ##
==========================================
- Coverage   83.86%   83.85%   -0.02%     
==========================================
  Files         556      557       +1     
  Lines        5857     5865       +8     
  Branches       12       12              
==========================================
+ Hits         4912     4918       +6     
- Misses        943      945       +2     
  Partials        2        2
Flag Coverage Δ
#patternfly3 84.89% <ø> (ø) ⬆️
#patternfly4 81.48% <66.66%> (-0.03%) ⬇️
#patternflymisc 95.65% <ø> (ø) ⬆️
Impacted Files Coverage Δ
...act-core/src/components/DataList/DataListAction.js 75% <66.66%> (ø)

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 1f12527...56e3b82. Read the comment docs.

karelhala
karelhala approved these changes Mar 14, 2019
View reviewed changes
Copy link
Contributor

@karelhala karelhala left a comment

Choose a reason for hiding this comment

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

I like this implementation! Simple and straight forward.

christiemolloy
christiemolloy approved these changes Mar 14, 2019
View reviewed changes
Copy link
Member

@christiemolloy christiemolloy left a comment

Choose a reason for hiding this comment

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

Thanks for applying the comments I made! Looks awesome

tlabaj
tlabaj approved these changes Mar 14, 2019
View reviewed changes
Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

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

LGTM

@dlabaj dlabaj merged commit 7636381 into patternfly:master Mar 14, 2019
@boaz0
Copy link
Member Author

boaz0 commented Mar 14, 2019

Thank you all for the patience and feedback. Appreciate it a lot.
🙇‍♂️ 🙏

@boaz0 boaz0 deleted the actions_datalist branch March 14, 2019 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dlabaj dlabaj dlabaj approved these changes

@tlabaj tlabaj tlabaj approved these changes

@mcoker mcoker Awaiting requested review from mcoker

+4 more reviewers

@mcarrano mcarrano mcarrano left review comments

@jgiardino jgiardino jgiardino left review comments

@karelhala karelhala karelhala approved these changes

@christiemolloy christiemolloy christiemolloy approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@tlabaj tlabaj

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.