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

Rename AccessibleSVG to SVG and make it work with React Native #9685

Merged
merged 3 commits into from Sep 28, 2018

Conversation

@gziolo
Member

gziolo commented Sep 7, 2018

Description

Follow-up: #9565.
Alternative to: #9294 and #9656.

TODO

  • Make it work with the mobile app (React Native).

How has this been tested?

npm test
npm run build - make sure all the SVG-based icons still work.

Types of changes

Refactoring.

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 added this to the 3.9 milestone Sep 7, 2018

@gziolo gziolo self-assigned this Sep 7, 2018

@gziolo gziolo requested a review from youknowriad Sep 7, 2018

@gziolo gziolo changed the title from Update/accessible svg rename to Rename AccessibleSVG to SVG and make it work with React Native Sep 7, 2018

@gziolo gziolo referenced this pull request Sep 7, 2018

Merged

React Native - SVG icons for Inserter #9294

0 of 4 tasks complete

@youknowriad youknowriad removed this from the 3.9 milestone Sep 13, 2018

@gziolo gziolo requested review from hypest and SergioEstevao Sep 25, 2018

@SergioEstevao

This comment has been minimized.

Show comment
Hide comment
@SergioEstevao

SergioEstevao Sep 25, 2018

Contributor
Contributor

SergioEstevao commented Sep 25, 2018

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Sep 25, 2018

Member

@SergioEstevao I brought back Native components for SVG, can you double check if it works properly now? I hope to continue alternative solution where you can write DOM tags for SVG body but have RN to convert them to proper components behind the scenes. If it doesn't work we will have to ensure that all SVG use the pattern established so far.

Member

gziolo commented Sep 25, 2018

@SergioEstevao I brought back Native components for SVG, can you double check if it works properly now? I hope to continue alternative solution where you can write DOM tags for SVG body but have RN to convert them to proper components behind the scenes. If it doesn't work we will have to ensure that all SVG use the pattern established so far.

@gziolo gziolo requested a review from mcsf Sep 25, 2018

## Usage
```jsx
import { G, Path, SVG } from '@wordpress/components';

This comment has been minimized.

@mcsf

mcsf Sep 25, 2018

Contributor

Polygon is missing; worth showing in the example?

@mcsf

mcsf Sep 25, 2018

Contributor

Polygon is missing; worth showing in the example?

This comment has been minimized.

@gziolo

gziolo Sep 26, 2018

Member

I don't think it's essential to be include. I don't even know what it translates to :)

@gziolo

gziolo Sep 26, 2018

Member

I don't think it's essential to be include. I don't even know what it translates to :)

@gziolo gziolo requested review from youknowriad and removed request for youknowriad Sep 26, 2018

@mcsf

mcsf approved these changes Sep 26, 2018

@gziolo gziolo requested a review from mzorz Sep 26, 2018

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Sep 26, 2018

Member

@mzorz, @hypest or @SergioEstevao, can you confirm that this PR works with wordpress-mobile/gutenberg-mobile#129?

Member

gziolo commented Sep 26, 2018

@mzorz, @hypest or @SergioEstevao, can you confirm that this PR works with wordpress-mobile/gutenberg-mobile#129?

@hypest

This comment has been minimized.

Show comment
Hide comment
@hypest

hypest Sep 26, 2018

Contributor

👋 @gziolo , I tested out wordpress-mobile/gutenberg-mobile#129 against this branch and, apart from some updating that RN side branch needs, the app actually errors because the paragraph block has the icon being defined using plain, lowercased svg elements.

EDIT: Well, I should add that there's an open PR #9294 for the changes/fixes already.

Contributor

hypest commented Sep 26, 2018

👋 @gziolo , I tested out wordpress-mobile/gutenberg-mobile#129 against this branch and, apart from some updating that RN side branch needs, the app actually errors because the paragraph block has the icon being defined using plain, lowercased svg elements.

EDIT: Well, I should add that there's an open PR #9294 for the changes/fixes already.

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Sep 28, 2018

Member

Okey, let's merge this one and rebase #9294 against master.

Member

gziolo commented Sep 28, 2018

Okey, let's merge this one and rebase #9294 against master.

@gziolo gziolo merged commit 5c2085a into master Sep 28, 2018

2 checks passed

codecov/project 48.83% (+<.01%) compared to 3f36aba
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gziolo gziolo added this to the 4.0 milestone Sep 28, 2018

@gziolo gziolo deleted the update/accessible-svg-rename branch Sep 28, 2018

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Oct 9, 2018

Contributor

Nice work following up here.

Contributor

mtias commented Oct 9, 2018

Nice work following up here.

@mzorz

This comment has been minimized.

Show comment
Hide comment
@mzorz

mzorz Oct 12, 2018

Contributor

Okey, let's merge this one and rebase #9294 against master.

#9264 merged 🎉

Contributor

mzorz commented Oct 12, 2018

Okey, let's merge this one and rebase #9294 against master.

#9264 merged 🎉

@mzorz mzorz referenced this pull request Oct 12, 2018

Merged

[RN mobile] fix className style in SVG primitive #10556

4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment