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

[RNMobile] Final Page Templates with localization #19525

Merged
merged 6 commits into from
Feb 20, 2020
Merged

Conversation

koke
Copy link
Contributor

@koke koke commented Jan 9, 2020

Waiting for #19106, rebase to master before it's ready for final review.

Description

This seems like the most sensible approach to template i18n for now. It would add some friction to creating and maintaining the templates, but we could create some tooling around that so we can create templates in Gutenberg, then export a JSON. It needs some comments for translators to clarify that it’s example content, but other than that this might be it

For wordpress-mobile/gutenberg-mobile#1473

How has this been tested?

  • Open the main app with metro running
  • Go to pages
  • Create a new page
  • Select a template
  • Expect: to see the modal with the preview
  • Tap on Apply
  • Expect: the content of the preview to be in the editor and the modal should close

Screenshots

About Contact
Portfolio Services
Team Team

Types of changes

Adds final set of page templates for mobile with their localization

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .

@koke koke added Internationalization (i18n) Issues or PRs related to internationalization efforts Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) labels Jan 9, 2020
@pinarol
Copy link
Contributor

pinarol commented Jan 10, 2020

It would be good to add more description about how to generate the json of the page in a practical way.

@koke koke force-pushed the add/spt-template-i18n branch from 87213dc to df44ae6 Compare January 24, 2020 11:14
@koke koke changed the base branch from add/spt-template-preview to master January 24, 2020 11:14
@koke koke force-pushed the add/spt-template-i18n branch from df44ae6 to da3dee5 Compare January 24, 2020 11:16
Gerardo Pacheco added 2 commits February 13, 2020 09:09
…template-i18n

# Conflicts:
#	packages/block-editor/src/components/page-template-picker/default-templates.js
@geriux geriux changed the title Page Template localization [RNMobile] Final Page Templates with localization Feb 13, 2020
@geriux geriux marked this pull request as ready for review February 13, 2020 10:34
@geriux
Copy link
Member

geriux commented Feb 13, 2020

Hey @iamthomasbishop ! Can you please review these emojis? I chose some random ones for Services and Team 😅

@geriux
Copy link
Member

geriux commented Feb 13, 2020

@koke I can't assign you as a reviewer because it's your PR 😄but whenever you have time, could you please check the added templates? I think they have the right format. Thanks!

@iamthomasbishop
Copy link

iamthomasbishop commented Feb 13, 2020

Hey @iamthomasbishop ! Can you please review these emojis? I chose some random ones for Services and Team 😅

Definitely! And we should probably start a spreadsheet to document which emoji/icon each layout uses 😄 Here's a start:

Layout Name Emoji name Emoji Info
Services Hammer and wrench 🛠 https://emojipedia.org/hammer-and-wrench/
Team Busts in silhouette 👥 https://emojipedia.org/busts-in-silhouette/
About Waving hand 👋 https://emojipedia.org/waving-hand/
Contact Envelope ✉️ https://emojipedia.org/envelope/
Blog Newspaper 📰 https://emojipedia.org/newspaper/
Portfolio Artist palette 🎨 https://emojipedia.org/artist-palette/

I'm not 100% sure if it matches what you've already got, feel free to correct me 😄

@geriux
Copy link
Member

geriux commented Feb 14, 2020

Thank you @iamthomasbishop !

Updated them:

Icons Icons

'https://a8ctm1.files.wordpress.com/2019/08/scatter-1.jpg?w=640',
link:
'https://a8ctm1.wordpress.com/portfolio/scatter-3/',
id: '658',
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 could be problematic. I imagine those IDs are for the site where the templates were created, but it could break things on any other site.

Also perhaps we should avoid the links as week, it's one thing that we are hot linking images from a demo site, but having them link to that site as well would be unexpected and confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interestingly enough, if I remove the links and ids from the template, when you try to add an image to that gallery, it replaces all the existing images. I'm not entirely sure if that's good or bad 😁

Copy link
Member

Choose a reason for hiding this comment

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

😅You're right, I've just tested that myself. Were there any discussions about these cases? Blocks that have media content: Gallery, Media & Text etc.?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not that I'm aware of, I think we didn't consider it could be an issue. I'd say removing the IDs and links is likely the least harmful option. It might be a bit confusing if they disappear when the user adds a new image, but they are meant to be placeholders, so maybe not that confusing? cc @pinarol @iamthomasbishop

Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we loose the IDs but keep the links? When we insert an image from a web url we don't have any ID at all, so it should be a valid case?

<!-- wp:image {"sizeSlug":"large"} -->
<figure class="wp-block-image size-large"><img src="https://a8ctm1.files.wordpress.com/2019/08/scatter-1.jpg" alt=""/></figure>
<!-- /wp:image -->

Copy link
Member

Choose a reason for hiding this comment

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

Why don't we loose the IDs but keep the links? When we insert an image from a web url we don't have any ID at all, so it should be a valid case?

This works! Forgot about linking images directly by URL, so there won't be any issues if we remove the IDs

Interestingly enough, if I remove the links and ids from the template, when you try to add an image to that gallery, it replaces all the existing images.

This looks like a bug? It should be happening independent from the SPTs

Hmm I don't know if its a bug or just an edge case. I tried to do that on the Web editor and the gallery removes the images when you add a new one as well. (Using this template without IDs)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok thanks for trying on web.

Copy link
Contributor

Choose a reason for hiding this comment

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

So do we have a consensus about moving fwd with only links? @koke @geriux Anything left to prevent merging this PR?

Copy link
Member

Choose a reason for hiding this comment

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

All good from my side =)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking good to me too

@iamthomasbishop
Copy link

Looking good, @geriux!

@koke
Copy link
Contributor Author

koke commented Feb 20, 2020

I can't approve since I'm the original author but :shipit: from me. I tested yarn genstrings and the new strings where there in the translation files.

@geriux
Copy link
Member

geriux commented Feb 20, 2020

I can't approve since I'm the original author but :shipit: from me. I tested yarn genstrings and the new strings where there in the translation files.

Thanks!!

@geriux geriux merged commit 5d8701c into master Feb 20, 2020
@geriux geriux deleted the add/spt-template-i18n branch February 20, 2020 12:55
@github-actions github-actions bot added this to the Gutenberg 7.6 milestone Feb 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internationalization (i18n) Issues or PRs related to internationalization efforts Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants