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

Blocks: Fix demo teaser markup, More block custom text #2828

Merged
merged 2 commits into from Oct 3, 2017

Conversation

Projects
None yet
3 participants
@aduth
Member

aduth commented Sep 28, 2017

This pull request seeks to resolve two issues with the More block:

  • Fix incorrect syntax for the demo content core/more block teaser. Currently, the teaser tag is treated as freeform content, causing a freeform block to be unintentionally inserted after the More tag in demo content.
  • Fix attribute naming for custom text (wrong: text, correct: customText)

Testing instructions:

Verify that when viewing the demo, the more block is shown with custom "Keep on reading!" text, that selecting the More block reveals "Teaser" is checked in the block settings, and that no freeform block is shown following the More block. Check that setting custom text to the More block, saving and reloading respects the changes made.

@aduth aduth added the Blocks label Sep 28, 2017

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Sep 28, 2017

Codecov Report

Merging #2828 into master will increase coverage by 0.01%.
The diff coverage is 62.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2828      +/-   ##
==========================================
+ Coverage   33.78%   33.79%   +0.01%     
==========================================
  Files         191      191              
  Lines        5692     5693       +1     
  Branches      997      998       +1     
==========================================
+ Hits         1923     1924       +1     
  Misses       3189     3189              
  Partials      580      580
Impacted Files Coverage Δ
blocks/api/serializer.js 98% <100%> (+0.08%) ⬆️
blocks/library/more/index.js 22.22% <25%> (-7.78%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2753f63...3ba6476. Read the comment docs.

codecov bot commented Sep 28, 2017

Codecov Report

Merging #2828 into master will increase coverage by 0.01%.
The diff coverage is 62.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2828      +/-   ##
==========================================
+ Coverage   33.78%   33.79%   +0.01%     
==========================================
  Files         191      191              
  Lines        5692     5693       +1     
  Branches      997      998       +1     
==========================================
+ Hits         1923     1924       +1     
  Misses       3189     3189              
  Partials      580      580
Impacted Files Coverage Δ
blocks/api/serializer.js 98% <100%> (+0.08%) ⬆️
blocks/library/more/index.js 22.22% <25%> (-7.78%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2753f63...3ba6476. Read the comment docs.

@youknowriad

I don't know if I'm doing something wrong but when refreshing the page, I lose the custom text (I see null) and the teaser option is not selected.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 29, 2017

Member

I don't know if I'm doing something wrong but when refreshing the page, I lose the custom text (I see null) and the teaser option is not selected.

Hm, yes, I can confirm this too. One half of this issue (not being serialized correctly) is fixed in 3a45620 .

But also, the "Null" text being shown stems from the fact that the PEG parser returns customText as null when not assigned, but our attribute sourcing only considers explicitly undefined in considering an attribute as "not assigned". Therefore, the null gets coerced to a string and shows verbatim. There are two options here:

Where this becomes more interesting is in how we align this with the PHP attributes behavior, where PHP only has the single concept of null, not null and undefined. In server-side rendering, we consider the null value as "not assigned". There is a distinction we can make in null vs isset in PHP, so perhaps we can emulate this by removing the null fallback in PHP and relying on isset to determine whether an attribute should apply the default value.

cc @dmsnell

Member

aduth commented Sep 29, 2017

I don't know if I'm doing something wrong but when refreshing the page, I lose the custom text (I see null) and the teaser option is not selected.

Hm, yes, I can confirm this too. One half of this issue (not being serialized correctly) is fixed in 3a45620 .

But also, the "Null" text being shown stems from the fact that the PEG parser returns customText as null when not assigned, but our attribute sourcing only considers explicitly undefined in considering an attribute as "not assigned". Therefore, the null gets coerced to a string and shows verbatim. There are two options here:

Where this becomes more interesting is in how we align this with the PHP attributes behavior, where PHP only has the single concept of null, not null and undefined. In server-side rendering, we consider the null value as "not assigned". There is a distinction we can make in null vs isset in PHP, so perhaps we can emulate this by removing the null fallback in PHP and relying on isset to determine whether an attribute should apply the default value.

cc @dmsnell

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 29, 2017

Member

In 8483b8a , I opted to update the PEG grammar to return falsey as undefined in the JavaScript implementation only, and for now not make any changes to either the PHP parse or treatment of attributes.

Member

aduth commented Sep 29, 2017

In 8483b8a , I opted to update the PEG grammar to return falsey as undefined in the JavaScript implementation only, and for now not make any changes to either the PHP parse or treatment of attributes.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 29, 2017

Member

Also, it is fun to see that the test fixtures illuminate the issue, but only after the fix is applied 😆 (See core__more__custom-text-teaser.serialized.html)

Member

aduth commented Sep 29, 2017

Also, it is fun to see that the test fixtures illuminate the issue, but only after the fix is applied 😆 (See core__more__custom-text-teaser.serialized.html)

Show outdated Hide outdated blocks/api/serializer.js Outdated
@dmsnell

dmsnell approved these changes Oct 3, 2017

thanks @aduth

@aduth aduth merged commit 01d1c09 into master Oct 3, 2017

3 checks passed

codecov/project 33.79% (+0.01%) compared to 2753f63
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@aduth aduth deleted the fix/post-content-teaser branch Oct 3, 2017

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