Skip to content

Override createElement to map svg elements to react-native-svg ones#11129

Closed
Tug wants to merge 7 commits intomasterfrom
try/map-svg-elements
Closed

Override createElement to map svg elements to react-native-svg ones#11129
Tug wants to merge 7 commits intomasterfrom
try/map-svg-elements

Conversation

@Tug
Copy link
Copy Markdown
Contributor

@Tug Tug commented Oct 26, 2018

Description

This PR reverts to using string versions of svg elements and introduces a custom createElement function for react-native which will be able to map those to native svg elements.
It will also be useful to have a custom createElement function in other case, such as having a failover for <div>, <span>...

How has this been tested?

To be tested along with wordpress-mobile/gutenberg-mobile#184
WIP

Types of changes

Build changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@gziolo gziolo mentioned this pull request Oct 26, 2018
Closed
@Tug Tug self-assigned this Oct 26, 2018
@Tug Tug added [Status] In Progress Tracking issues with work in progress [Type] Build Tooling Issues or PRs related to build tooling Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) labels Oct 26, 2018
@Tug Tug requested a review from gziolo October 26, 2018 16:44
gziolo
gziolo reviewed Oct 30, 2018
View reviewed changes
packages/block-library/src/code/index.js
description: __( 'Add text that respects your spacing and tabs -- perfect for displaying code.' ),

icon: <SVG viewBox="0 0 24 24" xmlns="http://www.w3.org/2000/svg"><Path d="M0,0h24v24H0V0z" fill="none" /><Path d="M9.4,16.6L4.8,12l4.6-4.6L8,6l-6,6l6,6L9.4,16.6z M14.6,16.6l4.6-4.6l-4.6-4.6L16,6l6,6l-6,6L14.6,16.6z" /></SVG>,
icon: <svg viewBox="0 0 24 24" xmlns="http://www.w3.org/2000/svg"><path d="M0,0h24v24H0V0z" fill="none" /><path d="M9.4,16.6L4.8,12l4.6-4.6L8,6l-6,6l6,6L9.4,16.6z M14.6,16.6l4.6-4.6l-4.6-4.6L16,6l6,6l-6,6L14.6,16.6z" /></svg>,
Copy link
Copy Markdown
Member

@gziolo gziolo Oct 30, 2018

Choose a reason for hiding this comment

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

Not sure about path, g and rest, but at least SVG needs to stay as it adds some accessibility features behind the scenes. It's expected to have it everywhere.

Copy link
Copy Markdown
Contributor Author

@Tug Tug Oct 30, 2018

Choose a reason for hiding this comment

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

Couldn't we just map svg to our SVG then (instead of the one from react-native-svg`)?

@gziolo gziolo added the [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible label Oct 30, 2018
@gziolo
Copy link
Copy Markdown
Member

gziolo commented Oct 30, 2018

Did you make it work? Changes in both PRs look reasonable. I will try to test it on Friday.

@Tug
Copy link
Copy Markdown
Contributor Author

Tug commented Oct 30, 2018

It works, but still needs some cleanup on the mobile PR (around how we import/build modules).
Also tests are failing + some conflicts already :)

@gziolo
Copy link
Copy Markdown
Member

gziolo commented Oct 30, 2018

Let me know when it is fully working, I will ping a few more developers to look closer into it.

The solution we have at the moment helped us to move forward, but I'm still not 100% confident it's bulletproof in the long run. Having this Eslint rule:
https://github.com/WordPress/gutenberg/blob/master/.eslintrc.js#L169-L183
makes it a bit problematic to use SVG and family on mobile inside save method. We don't do it at the moment so it's non issue, but I'm worried that we can't use any RN component inside save as they won't serialize to HTML.

@gziolo gziolo added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Jan 31, 2019
@gziolo
Copy link
Copy Markdown
Member

gziolo commented Jan 31, 2019

@Tug do you plan to continue working on this PR? It got very out of date :(

@gziolo gziolo added [Feature] UI Components Impacts or related to the UI component system and removed [Type] Build Tooling Issues or PRs related to build tooling labels Jan 31, 2019
@Tug
Copy link
Copy Markdown
Contributor Author

Tug commented Feb 5, 2019

It's not a priority at the moment no 👍
Will reopen if we want to come back to this again.

@Tug Tug closed this Feb 5, 2019
@Tug Tug deleted the try/map-svg-elements branch February 5, 2019 09:04
@Tug Tug restored the try/map-svg-elements branch February 28, 2019 11:57
@Tug Tug deleted the try/map-svg-elements branch November 21, 2019 17:44
@gziolo gziolo mentioned this pull request Mar 3, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gziolo gziolo gziolo left review comments

Assignees

@Tug Tug

Labels

[Feature] UI Components Impacts or related to the UI component system Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) [Status] In Progress Tracking issues with work in progress [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Tug @gziolo