feat(Indicator): add close indicator #1114
Conversation
mnajdova
requested review from
dvdzkwsk,
kuzhelov,
layershifter,
levithomason and
miroslavstastny
as code owners
| direction?: 'start' | 'end' | 'top' | 'bottom' | ||
|
|
||
| /** The indicator can point towards different directions. */ | ||
| code?: 'arrow' | 'close' |
There was a problem hiding this comment.
What if we will rename direction to variation/symbol and include there close?
/** The indicator can point towards different directions. */
- direction?: 'start' | 'end' | 'top' | 'bottom'
+ variation?: 'start' | 'end' | 'top' | 'bottom' | 'close'
- /** The indicator can point towards different directions. */
- code?: 'arrow' | 'close'There was a problem hiding this comment.
That was my first thought but the direction is more for how the indicator/icon should be rotated. For example if you have the arrow icon, it can be pointed to different directions... In that sense what will the close mean?
There was a problem hiding this comment.
Plus in rtl the directions for start and end are swapped...
There was a problem hiding this comment.
In the same the code below doens't have any sense:
<Indicator code='close' direction='start' />There was a problem hiding this comment.
An actually, it's more clear/cross than close because it doesn't closes anything in context of usage. May be it will be more obvious if we will prefix existing values with arrow- 🤔
<Indicator symbol='cross' />
<Indicator symbol='arrow-start' />There was a problem hiding this comment.
Then back to the other proposal we had (this component was introduced only because we don't have the set of icon which will be available in all themes). Can we then define in the typings of the themes, that the arrows and close icons will always be available and just use icon names directly? If we are okay with that, then we won't even need this component at all. @kuzhelov @miroslavstastny @layershifter
There was a problem hiding this comment.
Or we can define icons/symbols directly in styles of this component.
There was a problem hiding this comment.
I agree with @miroslavstastny that this component is just covering some missing parts from our API. If we have the icons we don't need any of that.. I would rather have the icons being required as those are required by the API of the components we have...
There was a problem hiding this comment.
+1 for @miroslavstastny 's comment and looks like we're going towards the right solution
It'd be good to define some sort of artifacts per themes in <key,value> pairs that can be reused in components styles; same as fontAwesome does for icons; not sure if we need more than icons at this point. It'd also be good to let users edit these
There was a problem hiding this comment.
Created alternative PR: #1120 please take a look
mnajdova
changed the title
|
Closing this in favor of #1120 |
Fixes #896