Skip to content

Conversation

@boaz0
Copy link
Member

@boaz0 boaz0 commented Jan 26, 2020

What: Closes #3537

//cc @mcoker @tlabaj

@patternfly-build
Copy link
Collaborator

patternfly-build commented Jan 26, 2020

@codecov-io
Copy link

codecov-io commented Jan 26, 2020

Codecov Report

Merging #3587 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3587      +/-   ##
==========================================
+ Coverage      71%   71.01%   +0.01%     
==========================================
  Files         785      785              
  Lines       10638    10643       +5     
  Branches     2314     2319       +5     
==========================================
+ Hits         7553     7558       +5     
  Misses       2655     2655              
  Partials      430      430
Flag Coverage Δ
#misc 95.45% <ø> (ø) ⬆️
#patternfly3 85.89% <ø> (ø) ⬆️
#patternfly4 60.04% <100%> (-39.96%) ⬇️
Impacted Files Coverage Δ
...tternfly-4/react-core/src/components/Card/Card.tsx 100% <100%> (ø) ⬆️
...s/patternfly-4/react-core/src/helpers/constants.ts 100% <100%> (ø) ⬆️
...on/src/components/CatalogTile/CatalogTileBadge.tsx 77.77% <0%> (ø) ⬆️
...components/CatalogItemHeader/CatalogItemHeader.tsx 66.66% <0%> (ø) ⬆️
...nfly-react/src/components/Pagination/Pagination.js 100% <0%> (ø) ⬆️
...atternfly-4/react-core/src/layouts/Level/Level.tsx 83.33% <0%> (ø) ⬆️
...e/src/components/Table/base/evaluate-formatters.ts 100% <0%> (ø) ⬆️
...-core/src/components/InputGroup/InputGroupText.tsx 80% <0%> (ø) ⬆️
...ct-core/src/components/LoginPage/LoginMainBody.tsx 100% <0%> (ø) ⬆️
...onents/ContextSelector/ContextSelectorMenuList.tsx 88.88% <0%> (ø) ⬆️
... and 411 more

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 60e7422...1ff1d71. Read the comment docs.

@tlabaj tlabaj requested a review from mcoker January 27, 2020 21:21
dlabrecq
dlabrecq previously approved these changes Jan 27, 2020
Copy link
Member

@dlabrecq dlabrecq left a comment

Choose a reason for hiding this comment

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

LGTM

@mcoker
Copy link
Contributor

mcoker commented Jan 27, 2020

thanks @boaz0! when isSelectable is applied, can we also add tabindex="0" so that you can tab to it? if it helps, the data-list has the same feature when a row is made selectable.

mcoker
mcoker previously approved these changes Jan 29, 2020
Copy link
Contributor

@mcoker mcoker left a comment

Choose a reason for hiding this comment

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

LGTM! @jessiehuff should this also work with a keyboard?

dlabrecq
dlabrecq previously approved these changes Jan 29, 2020
Copy link
Contributor

@jessiehuff jessiehuff left a comment

Choose a reason for hiding this comment

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

This looks good @boaz0 :) There is an issue with keyboard navigation though. If you add an action inside the card like a button or a link, when you select that action, the entire card becomes selected. I noticed that this is also an issue with our selectable data-list and created an issue for it. I think the event listener is stopping the event propagation.

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.

Can you add the new props to the demo-app and add integration test please.

@boaz0 boaz0 dismissed stale reviews from dlabrecq and mcoker via cd6008f January 31, 2020 09:55
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.

Could you also create a cypress test for the interaction.

tlabaj
tlabaj previously approved these changes Feb 3, 2020
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

dlabrecq
dlabrecq previously approved these changes Feb 3, 2020
Copy link
Member

@dlabrecq dlabrecq left a comment

Choose a reason for hiding this comment

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

I no longer see an issue navigating the kebab, and was able to select a card with the mouse. However, I could not select a card using just the keyboard.

Should users be able to select cards using either the enter key or the space bar?

Copy link
Contributor

@jessiehuff jessiehuff left a comment

Choose a reason for hiding this comment

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

Hey @boaz0, it's looking good so far! :) I just took a look at it again, and what's happening to Dan is also happening to me. Navigating and interacting with the action button works great, but I can't seem to find any way to select the card at all now. My thought is that pressing enter or space on the entire card/parent would select the card but pressing enter or space on the kebab/action would open the dropdown.

Signed-off-by: Boaz Shuster <boaz.shuster.github@gmail.com>
Copy link
Contributor

@jessiehuff jessiehuff left a comment

Choose a reason for hiding this comment

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

This looks awesome, @boaz0! It works in both keyboard and VO testing! 😄Great job! 🥇
Also, I see your point about the dropdown and love your idea captured in this issue. Would love your thoughts on the PR I created for it if you get a chance!

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

@tlabaj tlabaj merged commit 166aa09 into patternfly:master Feb 25, 2020
@boaz0 boaz0 deleted the closes_3537 branch February 25, 2020 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Card - add selectable/selected variation

7 participants