-
Notifications
You must be signed in to change notification settings - Fork 6
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
icon picker modal #2908
base: master
Are you sure you want to change the base?
icon picker modal #2908
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2908 +/- ##
==========================================
- Coverage 93.44% 93.10% -0.34%
==========================================
Files 414 416 +2
Lines 6436 6501 +65
Branches 438 439 +1
==========================================
+ Hits 6014 6053 +39
- Misses 341 367 +26
Partials 81 81
... and 6 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
import classnames from 'classnames'; | ||
import FaIcon from '../icon'; | ||
|
||
// eslint-disable-next-line css-modules/no-unused-class |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inutile
import FaIcon from '../icon'; | ||
|
||
// eslint-disable-next-line css-modules/no-unused-class | ||
import {COLORS} from '../../variables/colors'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ajoute le white ici pour pouvoir faire un COLOR.white et pas #ffffff en dessous
const ICON_COLOR = COLORS.cm_primary_blue; | ||
const CHECK_ICON_NAME_MAP = { | ||
single: 'circle-check', | ||
multi: 'square-check' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quel est l'application du multi check ? je ne le trouve pas das les design de werner
|
||
const {selectionMode, iconColor = ICON_COLOR} = options; | ||
|
||
const isSelected = selectionMode === 'single' || selectionMode === 'multi'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
weird comme logic: tu definit isSelected sur le mode de selection.
Ds un scenario real le mode de selection peut etre set sans qu'un item soit selectonné
De plus le mode de selection multiple, je ne vois pas l'interet mais c'est un autre pb...
tu devrais plutot avoir un truc comme ca : à minima. ou etre plus explicit sur ton intend here
const isSelected = options.isSelected ?? (selectionMode === 'single' || selectionMode === 'multi');
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isSelected est mm pas destructuré des options ici ... il devrait l'etre et default à false je pense.
17309cf
to
fe1266a
Compare
ticket
purpose: create icon picker modal
result:
storybook chromatic: https://6622682e70ecd8ecf5027fa1-asvmsschwg.chromatic.com/