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

Projects
None yet
3 participants
@miina
Copy link
Collaborator

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

This comment has been minimized.

Copy link
Member

westonruter commented May 21, 2018

Conflicts need resolving.

@miina

This comment has been minimized.

Copy link
Collaborator Author

miina commented May 21, 2018

Develop merged & conflicts resolved.

@westonruter
Copy link
Member

westonruter left a comment

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

This comment has been minimized.

Copy link
@westonruter

westonruter May 23, 2018

Member

Return is {Object}, no?

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

This comment has been minimized.

Copy link
@westonruter

westonruter May 23, 2018

Member

Needs translation

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

This comment has been minimized.

Copy link
@westonruter

westonruter May 23, 2018

Member

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

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

This comment has been minimized.

Copy link
@westonruter

westonruter May 23, 2018

Member

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 } );

This comment has been minimized.

Copy link
@westonruter

westonruter May 23, 2018

Member

Abort if nextMinFont is larger than maxFont

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

This comment has been minimized.

Copy link
@westonruter

westonruter May 23, 2018

Member

Should ampFitText be replaced with event.checked?

This comment has been minimized.

Copy link
@miina

miina May 23, 2018

Author Collaborator

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 } );

This comment has been minimized.

Copy link
@westonruter

westonruter May 23, 2018

Member

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

@miina

This comment has been minimized.

Copy link
Collaborator 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 and others added some commits May 23, 2018

Show error notices when amp-fit-text props are invalid
* Hide pixel unit since assumed for fonts.
* Fix reset behavior.
* Use number input type for height.
* Add default values for min and max font size.
@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented May 25, 2018

@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

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the add/amp-fit-text_support branch May 25, 2018

@pbakaus

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

Copy link
Member

westonruter commented May 25, 2018

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
You can’t perform that action at this time.