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

[Gutenberg] Add amp-fit-text support to text blocks #1151

Merged
merged 14 commits into from
May 25, 2018

Conversation

miina
Copy link
Contributor

@miina miina commented May 16, 2018

Adds amp-fit-text toggle and controls to the following blocks:

  • paragraph;
  • heading;
  • subheading;
  • code;
  • quote;

Settings when amp-fit-text is selected:
screen shot 2018-05-16 at 9 16 08 pm

Currently has the default layout of fixed-height, perhaps this could be changed with a next iteration once AMP Layout has been implemented.

@westonruter
Copy link
Member

Conflicts need resolving.

@miina
Copy link
Contributor Author

miina commented May 21, 2018

Develop merged & conflicts resolved.

Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

@miina When checking the fit-text checkbox, can the font size controls under Text Settings be hidden?

image

Also, can the same font-size-picker component be used for the min/max font? The default min could be the S size. And the default max could be the XL size.

I should think also that the max font size should not be able to set to a value greater than the height (if the max font size is set to be greater then perhaps the height could automatically be likewise increased). Likewise, I shouldn't think that the min font size should ever be able to be set to be larger than the max font size. I currently can do both of these things, and the result is an amp-fit-text that doesn't render.

*
* @param {Object} settings Settings.
* @param {string} name Block name.
* @return {*} Settings.
Copy link
Member

Choose a reason for hiding this comment

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

Return is {Object}, no?

TextControl = wp.components.TextControl,
ToggleControl = wp.components.ToggleControl,
PanelBody = wp.components.PanelBody,
label = 'Use AMP Fit Text';
Copy link
Member

Choose a reason for hiding this comment

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

Needs translation

props.setAttributes( { ampFitText: ! ampFitText } );
}
} ),
el( TextControl, {
Copy link
Member

Choose a reason for hiding this comment

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

Can the font-size-picker control be used instead?

label: __( 'Max font (px)' ),
value: maxFont,
onChange: function( nextMaxFont ) {
props.setAttributes( { maxFont: nextMaxFont } );
Copy link
Member

Choose a reason for hiding this comment

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

Make sure the height is set to be the Math.max() of the nextMaxFont and its current value.

label: __( 'Min font (px)' ),
value: minFont,
onChange: function( nextMinFont ) {
props.setAttributes( { minFont: nextMinFont } );
Copy link
Member

Choose a reason for hiding this comment

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

Abort if nextMinFont is larger than maxFont

label: label,
checked: ampFitText,
onChange: function() {
props.setAttributes( { ampFitText: ! ampFitText } );
Copy link
Member

Choose a reason for hiding this comment

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

Should ampFitText be replaced with event.checked?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of the core blocks seem to use negating the current value for toggle controls (as far as I've seen), thought of staying consistent with the core blocks when using that. Thoughts?

label: label,
checked: ampFitText,
onChange: function() {
props.setAttributes( { ampFitText: ! ampFitText } );
Copy link
Member

Choose a reason for hiding this comment

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

Can this also result in the font-size-picker and range-control under Text Settings to be hidden when ampFitText is true?

@miina
Copy link
Contributor Author

miina commented May 23, 2018

@westonruter Changes made. It's still possible to set larger minFont than maxFont, for example when first setting minFont to 30 and then setting maxFont to something lower. However, adding the check for aborting maxFont change when it's lower than minFont would make editing the values very unfriendly for the user. E.g. if maxFont and minFont are both 15 and the user would like to set maxFont to 25, then removing 5 from 15 wouldn't be possible since then the temporary 1 would be lower than the minFont value.

Would be great if the InspectorControls would have validation option (for example with option to display error message) and disable saving in case of incorrect values instead of automatically changing other values. Wondering if something like this is planned, tried to find an issue for it but without success.

@westonruter
Copy link
Member

@pbakaus with 937035f I've made it so that if you are using an AMP block in content that the required AMP component scripts will get added to the page even in non-AMP documents. This ensures that the components actually render instead of being empty, which seems better. The downside here is that the AMP runtime is trying to take control of the entire page, not just the components you're using. So for example attempting to submit the comment form results in an error:

image

I think that the AMP runtime should perhaps skip doing anything other than render specific components when the document lacks the boilerplate or the amp attribute on the html element. So that the AMP components can be better used as a component library. Would this be “dirty AMP”?

@westonruter westonruter added this to the v1.0 milestone May 25, 2018
@westonruter westonruter merged commit 8bdc1b3 into develop May 25, 2018
@westonruter westonruter deleted the add/amp-fit-text_support branch May 25, 2018 00:14
@pbakaus
Copy link

pbakaus commented May 25, 2018

@westonruter good call. Yes, I think it's fair to file this on the amphtml repo as issue, should be a fairly easy fix.

@westonruter
Copy link
Member

I think it's fair to file this on the amphtml repo as issue

ampproject/amphtml#15583

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants