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(DataList): add selectable variant #3404

Merged
merged 3 commits into from Dec 17, 2019

Conversation

@nicolethoen
Copy link
Contributor

nicolethoen commented Dec 12, 2019

addresses #3233

I have not figured out a clean way to prevent clicking the kebab (or interacting with components which have their own event handlers) from also selecting a DataListItem row. Events keep propagating up to parent components. I'd love any help people are willing to give... (@evwilkin @jschuler).

Is it possible the interaction as is would be acceptable for merging before code freeze monday? I could open an issue to figure out the last part? @mcarrano @mcoker

@nicolethoen nicolethoen force-pushed the nicolethoen:hover_list_view branch from 995501a to cd84463 Dec 12, 2019
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Dec 12, 2019

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

@mcarrano

This comment has been minimized.

Copy link
Member

mcarrano commented Dec 12, 2019

@nicolethoen without actually seeing this, my opinion is that it would be a significant usability problem if opening a kabob, for example also selected the row. If we were to release this, I would only do so with the restriction that the row should not include active elements to avoid this behavior. @LHinson @rachael-phillips do you recall who was asking for this feature and what their intended usage was?

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 12, 2019

Codecov Report

Merging #3404 into master will decrease coverage by 0.01%.
The diff coverage is 64.1%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3404      +/-   ##
==========================================
- Coverage   67.12%   67.11%   -0.02%     
==========================================
  Files         903      903              
  Lines       25445    25469      +24     
  Branches     2243     2247       +4     
==========================================
+ Hits        17081    17093      +12     
- Misses       7333     7342       +9     
- Partials     1031     1034       +3
Flag Coverage Δ
#misc 95.45% <ø> (ø) ⬆️
#patternfly3 69.29% <ø> (ø) ⬆️
#patternfly4 64.23% <64.1%> (-0.03%) ⬇️
Impacted Files Coverage Δ
...eact-core/src/components/DataList/DataListItem.tsx 56.66% <51.85%> (-35%) ⬇️
...-4/react-core/src/components/DataList/DataList.tsx 93.33% <91.66%> (+4.44%) ⬆️

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 222e0d7...4d1898e. Read the comment docs.

@tlabaj tlabaj added the css review label Dec 13, 2019
@dlabaj

This comment has been minimized.

Copy link
Contributor

dlabaj commented Dec 16, 2019

@nicolethoen Can you do a prevent default when you click the kebab?

@nicolethoen nicolethoen force-pushed the nicolethoen:hover_list_view branch from cd84463 to 3a4326d Dec 16, 2019
@nicolethoen nicolethoen changed the title [WIP] feat(DataList): add selectable variant feat(DataList): add selectable variant Dec 16, 2019
);
export class DataListItem extends React.Component<DataListItemProps> {

// @ts-ignore

This comment has been minimized.

Copy link
@tlabaj

tlabaj Dec 16, 2019

Contributor

Why do we need this?

This comment has been minimized.

Copy link
@nicolethoen

nicolethoen Dec 17, 2019

Author Contributor

typescript has some issues with pulling in context this way. For a handful of versions of React, pulling in a static context was a deprecated practice. It is now supported again, but the ts linter doesn't know that? All instances of a static context in the project need the ts-ignore flag. And its the only way to reference the context outside of a context.consumer in a render function.

Copy link
Contributor

tlabaj left a comment

You also need to update the demo-app and integration test.

@nicolethoen nicolethoen dismissed stale reviews from rebeccaalpert and dlabaj via 953c88e Dec 17, 2019
@nicolethoen nicolethoen force-pushed the nicolethoen:hover_list_view branch from 3a4326d to 953c88e Dec 17, 2019
Copy link
Contributor

mcoker left a comment

LGTM! Thanks @nicolethoen!

The only part I'm unsure about is the transition/fade of the left border when you select an item. That's because the expanded state left border fades when you expand/collapse a row, and the selectable row uses the same border to indicate the selected state.

@mcarrano what do you think? Would you expect the left border to fade in/out when you select/unselect a row?

@nicolethoen nicolethoen force-pushed the nicolethoen:hover_list_view branch from 953c88e to 4d1898e Dec 17, 2019
@nicolethoen

This comment has been minimized.

Copy link
Contributor Author

nicolethoen commented Dec 17, 2019

I rebased with the latest changes, but now selecting the rows doesn't appear to be working. let me investigate quickly

@nicolethoen

This comment has been minimized.

Copy link
Contributor Author

nicolethoen commented Dec 17, 2019

I rebased with the latest changes, but now selecting the rows doesn't appear to be working. let me investigate quickly

I take it back! It looks like the example is working in the surge preview :) @mcarrano @mcoker @tlabaj

@tlabaj
tlabaj approved these changes Dec 17, 2019
Copy link
Contributor

tlabaj left a comment

LGTM

@dlabaj
dlabaj approved these changes Dec 17, 2019
@mcoker
mcoker approved these changes Dec 17, 2019
Copy link
Member

mcarrano left a comment

Looks good now @nicolethoen .

Regarding the question from @mcoker here:

Would you expect the left border to fade in/out when you select/unselect a row?

I am OK with this behavior.

@tlabaj tlabaj merged commit eeacf0c into patternfly:master Dec 17, 2019
9 checks passed
9 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_a11y_pf4 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 Dec 17, 2019

Your changes have been released in:

  • @patternfly/react-catalog-view-extension@1.1.57
  • @patternfly/react-core@3.129.0
  • @patternfly/react-docs@4.16.69
  • @patternfly/react-inline-edit-extension@2.14.17
  • demo-app-ts@3.16.0
  • @patternfly/react-integration@3.16.0
  • @patternfly/react-table@2.24.61
  • @patternfly/react-topology@2.11.45
  • @patternfly/react-virtualized-extension@1.3.58

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
8 participants
You can’t perform that action at this time.