Save/output/preview templates with units and wrappers.

[dvoytenko]

Fixes #198.

Follow up on #136 to apply units and wrappers to save/output/wrapper templates.

Forked from ampproject/amp-wp#4173

[dvoytenko]

@miina, @swissspidy, this is the first draft that I wanted to see if there are any major concerns that you see. I'm still testing it and cleaning up. But here are the major themes:

1. I actually found that the Preview template is much more similar to Display. So much so, that I'm now completely reusing DisplayElement and Display templates for everything. There might be a few places where we might want to override the Display template. But in majority of cases it works!
2. I renamed Save to Output. Like all other templates it works by using Wrapper approach with size/position and the content is filled inside.
3. There are some major issues with relative font-size. I found a solution, but it relies on a rather new CSS features. I've sent this case to the Stories team and we'll probably add something on the runtime level to make it simpler.
4. I also added the 9:16 safe area in the output. That's actually the reason why there are now issues with the font-size. But IMHO it's better to get it addressed sooner.

[miina]

@dvoytenko

1. I actually found that the Preview template is much more similar to Display. So much so, that I'm now completely reusing DisplayElement and Display templates for everything.

Makes sense to me, this was considered initially as well but at that point Output and Preview seemed to be more similar, this has changed with the refactoring work. Display doesn't include the AMP attributes and elements as the Output does so in that sense also seems to be a better option.

Q: Didn't see any preview related code in the PR even though it's mentioned in the comment -- is this still an idea/local only or forgot to push?

Same for the 9:16 safe area -- it's currently in the Output, however, for saving, it looks like at this moment Save is still being used. Let me know if I'm missing something or if it's currently expected.

Generally, the points make sense to me. Looking forward to hearing feedback about the responsive font size from the Stories team.

[dvoytenko]

Q: Didn't see any preview related code in the PR even though it's mentioned in the comment -- is this still an idea/local only or forgot to push?

Yep! Forgot to push. Sorry about that.

Generally, the points make sense to me. Looking forward to hearing feedback about the responsive font size from the Stories team.

I think currently this PR has a good take on font-size. But we simply do not have enough variables to construct it in a browser-safe way. So that would require the runtime team to either change slightly the meaning of the --story-page-v* or to add another var. But I think there's enough precedent do it either, so I hope this won't be an issue.

But to clarify, this is how the conversion works now:

• Element wrapper's width and height are relative to the top container's (9:16) width and height and expressed universally in %. E.g. width = dataWidth / PAGE_WIDTH * 100% and the same for height. This works as before and it's great since we don't really depend on the actual pixel values of container's width and height.
• Font can't work like this. A % value is relative to the container's parent height. To make it as similar as possible to width/height, I'm now setting the top level container's fontSize to the container's height. E.g. if height is 535px, the fontSize=535px. This way, the element's fontSize is expressed exactly the same way as height and fontSize = dataFontSize / PAGE_HEIGHT * 100%.
• The width/height and other length values inside an element (such as image's focal offset) are recalculated to be relative to the element wrapper's width/height, not the top-level container's width/height.

[dvoytenko]

@miina @swissspidy this is ready for review

[miina]

Should this (the min()) be addressed already now? It looks like it's not supported in Firefox at all which would likely mean the font not being displayed. Thoughts?

[dvoytenko]

I think this will just have to be wrong for now until we get some assistance from the Runtime team to make it cross-browser.

[miina]

Note from testing:

Currently, the font size and also width/height displayed in the sidebar is showing unexpected numbers for the user, for example:

Should there be an additional conversion for displaying the numbers in the sidebar that would reflect the actual sizes in the editor better?

[dvoytenko]

Should there be an additional conversion for displaying the numbers in the sidebar that would reflect the actual sizes in the editor better?

These are "data pixels". They are based on the 1080:1920. They do not depend on the size of the page card. I.e. if the window is resized smaller, and the page area gets smaller, the measurements in the side panel do not change. IMHO, 1080:1920 it's a pretty big base and we might change it later. But on the other side, I think, the idea is that people don't pay a lot of attention to these numbers outside just understanding their relative nature.

[miina]

These are "data pixels". They are based on the 1080:1920. They do not depend on the size of the page card. I.e. if the window is resized smaller, and the page area gets smaller, the measurements in the side panel do not change. IMHO, 1080:1920 it's a pretty big base and we might change it later. But on the other side, I think, the idea is that people don't pay a lot of attention to these numbers outside just understanding their relative nature.

According to Figma, the user can change width/height in numbers and also the font size. Should then the numbers not be displayed to the user instead? E.g. the user can increase/decrease the font/width/height without seeing the numbers behind?

I understand these are "data pixels", however, thinking of a user who might find a huge font size number unexpected. Thoughts?

[dvoytenko]

I understand these are "data pixels", however, thinking of a user who might find a huge font size number unexpected. Thoughts?

Yeah, I think it's definitely possible. And I think UX is still looking at this. But not sure if they have good options. IMHO we should go back to the smaller "data space", e.g. 412:732. But UX will be probably experimenting with it for a bit. Our most commonly intended page area size is 412:732 and I'm not sure going so far above it is very useful. However, development-wise, 1080:1920 at this point is actually very helpful. I fixed a number of issues where I missed relative conversion because it was visually harder to notice at 412:732 and became very obvious at 1080:1920.

[miina]

Ah, alright, then we can just wait for the UX on this and adjust later if needed. 👍

[dvoytenko]

Ah, alright, then we can just wait for the UX on this and adjust later if needed. 👍

Yep. Thus this is quite independent from this PR. And the new migration tools can easily help us transition to the next system w/o disruption like this: https://github.com/google/web-stories-wp/blob/master/assets/src/edit-story/migration/migrations/v0002_dataPixelTo1080.js

[miina]

Thus this is quite independent from this PR. And the new migration tools can easily help us transition to the next system w/o disruption like this

Alright, sounds good.

I created some merge conflicts now.

[dvoytenko]

@miina the pr is rebased with your latest changes.

[miina]

LGTM