feat(selectionCard): update states and spacing in selection card #2240
Conversation
samyak3009
force-pushed
the
feat-selection-card-states
branch
from
ca173cb to
14f2b26
Compare
samyak3009
force-pushed
the
feat-selection-card-states
branch
4 times, most recently
from
44c5058 to
471830c
Compare
| @@ -41,20 +41,22 @@ export const SelectionCard = (props: SelectionCardProps) => { | |||
|
|
|||
| const classes = classNames( | |||
| { | |||
| ['Selection-card']: true, | |||
| ['Selection-card--selected']: selected, | |||
| ['Selection-card']: !disabled, | |||
There was a problem hiding this comment.
Please create a separate class for non-disabled states such as Selection-card--default. Selection-card here should act as a generic class with common css properties for both the default and disabled state.
css/src/components/selectionCard.css
Outdated
| border-radius: var(--spacing-m); | ||
| position: relative; | ||
| cursor: not-allowed; | ||
| box-shadow: inset 0 0 0 var(--spacing-xs) var(--secondary-lighter); | ||
| transition: var(--duration--fast-01) var(--standard-productive-curve); |
There was a problem hiding this comment.
Instead of repeating the css properties in both Selection-card and Selection-card--disabled please move all the common properties into one css class
css/src/components/selectionCard.css
Outdated
| border-radius: var(--spacing-m); | ||
| position: relative; | ||
| cursor: not-allowed; | ||
| box-shadow: inset 0 0 0 var(--spacing-xs) var(--primary-lighter); | ||
| transition: var(--duration--fast-01) var(--standard-productive-curve); |
There was a problem hiding this comment.
Please refactor this as well according to above mentioned comments.
|
Please check the following use-cases once from design end:
|
|
Please check this once if we need a separate |
|
Please write a test case ensuring that user is not able to select/deselect disabled cards. Write test case for both single-select and multi-select cards. |
samyak3009
force-pushed
the
feat-selection-card-states
branch
4 times, most recently
from
3423c6f to
a042b48
Compare
| const element3 = getByTestId('Selection-card-3'); | ||
| fireEvent.click(element3); | ||
| expect(element3).not.toHaveClass('Selection-card--selected'); |
There was a problem hiding this comment.
Please check this test case once as on selection of element3 it should have Selection-card--selected class present. This test contradicts with the expected behavior. Please check how this test case is passing
There was a problem hiding this comment.
@anuradha9712 element3 is a disabled card so it must not be selected if it get clicked.
| <SelectionCard | ||
| id="item1" | ||
| data-test="Selection-card-3" | ||
| disabled={true} | ||
| onClick={() => updateCardSelection('item1')} | ||
| selected={isCardSelected('item1')} | ||
| > | ||
| Single Selection Item 3 | ||
| </SelectionCard> |
There was a problem hiding this comment.
Please remove item1 with item3
| <SelectionCard | ||
| id="item1" | ||
| data-test="Selection-card-3" | ||
| disabled={true} | ||
| onClick={() => updateCardSelection('item1')} | ||
| selected={isCardSelected('item1')} | ||
| > | ||
| Single Selection Item 3 | ||
| </SelectionCard> |
There was a problem hiding this comment.
Please remove item1 with item3
samyak3009
force-pushed
the
feat-selection-card-states
branch
from
a042b48 to
ec5e317
Compare
samyak3009
force-pushed
the
feat-selection-card-states
branch
from
ec5e317 to
8ea2982
Compare
What does this implement/fix? Explain your changes.
...
Does this close any currently open issues?
...
Any other comments?
...
Dependent PRs/Commits
...
Describe breaking changes, if any.
...
Checklist
Check all those that are applicable and complete.
masterbranch