dojo / widgets

:rocket: Dojo - UI widgets.
https://widgets.dojo.io
Other
88 stars 66 forks source link

fix: rename CardProperties.onAction to onContentClick #1705

Closed bitpshr closed 3 years ago

bitpshr commented 3 years ago

Type: feature

The following has been addressed in the PR:

Description:

This pull request renames the Card prop onAction to onContentClick, since it more-accurately describes what the prop actually does.

Resolves #1648

vercel[bot] commented 3 years ago

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

widget-test-docs – ./

🔍 Inspect: https://vercel.com/dojo/widget-test-docs/6oPDmRkRjvuEuPXFgk1bJSpXP9qN
✅ Preview: https://widget-test-docs-git-fork-bitpshr-fix-card-prop-name-dojo1.vercel.app

dojo.widgets – ./

🔍 Inspect: https://vercel.com/dojo/dojo.widgets/9LuzV1ByLqtaUu8bbLTKLzezX7Ub
✅ Preview: https://dojowidgets-git-fork-bitpshr-fix-card-prop-name-dojo1.vercel.app

codecov[bot] commented 3 years ago

Codecov Report

Merging #1705 (41881cb) into master (ce182a0) will increase coverage by 0.11%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1705      +/-   ##
==========================================
+ Coverage   89.96%   90.07%   +0.11%     
==========================================
  Files          94       94              
  Lines        5031     5047      +16     
  Branches     1364     1375      +11     
==========================================
+ Hits         4526     4546      +20     
  Misses        249      249              
+ Partials      256      252       -4     
Impacted Files Coverage Δ
src/card/index.tsx 100.00% <100.00%> (ø)
src/text-input/index.tsx 92.68% <0.00%> (-0.24%) :arrow_down:
src/tooltip/index.tsx 77.41% <0.00%> (ø)
src/date-input/index.tsx 91.07% <0.00%> (ø)
src/time-picker/index.tsx 81.81% <0.00%> (ø)
src/password-input/index.tsx 78.57% <0.00%> (ø)
src/chip-typeahead/index.tsx 89.71% <0.00%> (+0.09%) :arrow_up:
src/select/index.tsx 86.95% <0.00%> (+0.86%) :arrow_up:
src/list/index.tsx 79.94% <0.00%> (+2.16%) :arrow_up:

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 ce182a0...41881cb. Read the comment docs.

tomdye commented 3 years ago

I didn't even know there was a click handler for card content, should it even be there? As there aren't any more click handlers for the Card, do you think onClick would be more appropriate name wise? I think that was the suggestion in the issue too.

bitpshr commented 3 years ago

@tomdye onClick seems wrong since this does not include the card header, which I'd expect as a consumer. I think we should either call it onContentClick, or remove it altogether.

bitpshr commented 3 years ago

This pull request has been updated in the following ways:

  1. onClick is used instead of onContentClick, and this applies to all content but the action buttons as per the Material guidelines. See Action Area 1 in this image (reference.)
  2. Styling was improved for header content so it has proper padding.
tomdye commented 3 years ago

@bitpshr could you add an example using the click handler please and add the pointer: cursor css class we discussed in the dojo call when an onClick callback as been passed.