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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 7 commits into from
Closed

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

wants to merge 7 commits into from

Conversation

Tug
Copy link
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
@@ -20,7 +16,7 @@ export const settings = {

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
Member

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
Contributor Author

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
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
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
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
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
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