-
Notifications
You must be signed in to change notification settings - Fork 55
fix(Accordion, Menu): consilidating unicode arrow characters across the components #673
Conversation
We will create shorthands for the icon for these, but as the icons are theme dependent, unicode characters should be the default value. The 'chevron' icon might not exists in some themes. If we add some basic icons in the base theme, which is now under development, then we can safely replace the unicode characters with some icon, but still for the themes that will be implemented from scratch this could still be a problem. Until we decide on this points, adding the same unicode characters and icon shorthands should be enough for covering all points. |
@@ -66,7 +66,7 @@ class AccordionTitle extends UIComponent<ReactProps<AccordionTitleProps>, any> { | |||
|
|||
return ( | |||
<ElementType {...rest} className={classes.root} onClick={this.handleClick}> | |||
{active ? <span>▼</span> : <span>▶</span>} | |||
{active ? <span>▾</span> : <span>▸</span>} |
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.
Can we move the Unicode characters under some variables with descriptive naming?
for e.g. const arrowDown = '▾'
If firstly look at code without running, can be a "guess" game to know what does it represent :)
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.
I moved all usages of the unicode characters in the styles and added constants in the teams theme.
Oleksandr had a nice idea about making those arrows non-selectable in my Dropdown PR. Maybe it will be useful to the arrows in this PR as well. https://developer.mozilla.org/en-US/docs/Web/CSS/user-select |
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.
Look at the user-select link from Shift.
All comments resolved. I moved all unicode characters usages in the teams theme, and they can be safely move later in the base theme. In addition to this, we can add icon shorthands on all components. Let me know if you have other suggestions. |
src/themes/teams/constants.ts
Outdated
@@ -0,0 +1,8 @@ | |||
const constants = { |
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.
I see these constants as part of the base theme. What do you think? Should we add it there now or later on, when individual components are going to have their base theme implementations?
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.
I added initially the constants file as I was using this in the components' files. Now that we are using this here, they might become part of the siteVariables, I don't think there will be need for additional constants... For sure, the styles responsible for the arrows will go in the base theme.
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.
As for the question of this specific unicode character, my opinion is that we should add them in the base theme, once we move one of these components' styles there. This arrow should create the very basic look of the arrows (not using any specific icons) which is exactly what our base theme is. Later we will add the icon slots, and each theme can further customize this.
-removed arrow slot from the AccordionTitle
// | ||
// UNICODE CHARACTERS | ||
// | ||
export const arrowRight = '\u25B8' |
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.
This is not RTL-aware. Please create a tracking issue before merging this PR.
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.
Will do, thanks for noticing.
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.
Here it is for refenrence: #684
# Conflicts: # CHANGELOG.md
We have inconsistency of using the unicode arrow characters across different components: Accordinon, Menu, Dropdown etc.
Accordion
Menu
Dropdown
The problem on the Dropdown is slightly different, as we are using icon name, that may not even be defined in some themes. (see comment https://github.com/stardust-ui/react/pull/584/files#r244750605)
This PR fixes this inconsistency for the Accordion and Menu components (as there is already started work on the Dropdown for handling the toggle button, I propose that it should be fixed in that PR).
This is the look of the components after the changes:
Accordion
Menu: