Skip to content
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

Metadata for A through B, removed some LTR/RTL folders, and merged in… #604

Closed
wants to merge 4 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jul 12, 2023

Metadata for A through B, removed some LTR/RTL folders, and merged Book and Book Default.
Issue: #603
Folders removed:

Todo:
Re-export Arrow Export.

  • Arrow Export LTR => Arrow Export
  • Arrow Export RTL
  • Arrow Repo Temp LTR
  • Arrow Repo Temp RTL
  • Arrow Undo Temp RTL
  • Arrow Undo Temp LTR
  • Book Default (merged into Book)

Copy link
Collaborator

@behowell behowell left a comment

Choose a reason for hiding this comment

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

I don't think we can remove or rename the existing LTR/RTL icons, such as "Arrow Export LTR"/"Arrow Export RTL", as that will break anyone who's currently using them. We could remove them in a future major version of the icon package, but not at this time.

Instead, I think it will be necessary to keep the existing LTR and RTL variants of each icon, without "directionType": "mirror".

Then copy the LTR version to one without a suffix with "directionType": "mirror". (E.g. copy "Arrow Export LTR" to Arrow Export", with mirroring).

@spencer-nelson
Copy link
Member

The purpose of this work is explicitly to remove the RTL/LTR and will be a major change anyway. For the most part, the major versions to these libraries have already been thrown around enough with React having resizable icons and no other platform following up, iOS having language directionality support (which variance highly contributed to this particular need to remove files) and fonts being tied/adopted for some platforms but not others. Hence, should we just consider this directionality to be a breaking change anyway and deal with the tech debt of removal now, rather than continuing to punt it down the road?

@behowell
Copy link
Collaborator

@spencer-nelson It would be ideal if we could add the auto-mirroring support without a major version bump, and then later delete the redundant icons with a major version.

From an engineering perspective, partners picking up this change will need time to update their code to no longer refer to the LTR/RTL icons. By adding the new mirroring functionality separately before removing the existing icons, we give them a chance to do that work over time, rather than requiring it to be done immediately.

Also, do you know if all consumers are able to auto-mirror the icons based on the metadata? There might still be a longer-term need for the separate RTL/LTR variants in places that don't (yet) support mirroring.

@ghost
Copy link
Author

ghost commented Aug 25, 2023

Closing, addressed feedback in #618.

@ghost ghost closed this Aug 25, 2023
@ghost ghost deleted the williamch/metadataAthroughB branch August 25, 2023 16:59
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants