Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

feat(Icon): mark icons that should be rotated in rtl for teams theme #788

Merged
merged 15 commits into from
Jan 30, 2019

Conversation

mnajdova
Copy link
Contributor

@mnajdova mnajdova commented Jan 29, 2019

This PR adds mechanism for marking icon definitions that should rotate in rtl. Also the following icons are already marked and will be rotated in rtl:

  • bullets
  • indent
  • leave
  • outdent
  • redo
  • send
  • undo

LTR:
image

RTL:
image

Question:

How should we handle icons that have text or numbers? For example the numbers of the number-list icon in rtl are mirrored, which is not something we want... Any ideas will be appreciated. - Will be resolved in another PR.

@@ -4,29 +4,29 @@ import { Icon, Label } from '@stardust-ui/react'
const IconExampleSpace = () => (
<div>
<p>
<Label content="Default" />
<Label>Default</Label>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was change because of console errors for invalid dom structure (div inside p element)

Copy link
Member

@notandrew notandrew Jan 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is <Label> referring to an HTML element or a Stardust element?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@miroslavstastny
Copy link
Member

How should we handle icons that have text or numbers? For example the numbers of the number-list icon in rtl are mirrored, which is not something we want... Any ideas will be appreciated.

I think that for these special cases, the Icon component must render different SVG paths in RTL.

@mnajdova
Copy link
Contributor Author

How should we handle icons that have text or numbers? For example the numbers of the number-list icon in rtl are mirrored, which is not something we want... Any ideas will be appreciated.

I think that for these special cases, the Icon component must render different SVG paths in RTL.

@miroslavstastny thanks for your feedback. Do we want to support this somehow, or should this be user's responsibility then? We may add mapping icon name for RLT for this special icons...

CHANGELOG.md Outdated Show resolved Hide resolved
src/themes/teams/components/Icon/iconStyles.ts Outdated Show resolved Hide resolved
src/themes/teams/components/Icon/svg/types.d.ts Outdated Show resolved Hide resolved
src/themes/teams/withProcessedIcons.ts Outdated Show resolved Hide resolved
@kuzhelov
Copy link
Contributor

@miroslavstastny thanks for your feedback. Do we want to support this somehow, or should this be user's responsibility then? We may add mapping icon name for RLT for this special icons...

would suggest just to pass rtl value to icon's path rendering logic (currently we are just passing classes there) - and then it will be up to the client's logic to decide how this case should be processed

@mnajdova
Copy link
Contributor Author

would suggest just to pass rtl value to icon's path rendering logic (currently we are just passing classes there) - and then it will be up to the client's logic to decide how this case should be processed

@kuzhelov will created this as other PR once we are certain what we want to do in such cases.

Copy link
Member

@layershifter layershifter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, only one thing disturbs me, do we have a better name than rotateInRtl? 💭

@mnajdova mnajdova merged commit 8435577 into master Jan 30, 2019
@layershifter layershifter deleted the feat/rtl-icons-teams-theme branch January 30, 2019 14:52
@codepretty
Copy link
Collaborator

Is there a case where we have one icon that is flipped in some cases, but not others and it just depends on context? I was reading through Material Design's bidirectionality docs and it appears like it will be based on the icon...?

@codepretty
Copy link
Collaborator

How should we handle icons that have text or numbers? For example the numbers of the number-list icon in rtl are mirrored, which is not something we want... Any ideas will be appreciated.

I think that for these special cases, the Icon component must render different SVG paths in RTL.

I agree. We have icons that will use different paths for different sizes for example. It would be great to think about both cases when you implement a solution for showing different paths for rtl.

@mnajdova
Copy link
Contributor Author

@codepretty currently for the icons we are providing just the classes, but if we realize that we need more context there (props, theme etc), we can add these things. Let's just go step by step and add them only when necessary.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants