CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Time To Read: Add a range option #71606
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
Conversation
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
1 similar comment
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @wesleyfantinel. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Size Change: +335 B (+0.02%) Total Size: 1.95 MB
ℹ️ View Unchanged
|
Flaky tests detected in d5ce391. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/17919129194
|
const AVERAGE_READING_RATE = 189; | ||
const MIN_READING_RATE = 138; | ||
const MAX_READING_RATE = 228; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we remove Average Reading Rate and rely on MIN + MAX / 2
to determine a central point? In this case, it would become 183 instead of 189.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, we could leave AVERAGE_READING_RATE, but have it be a customizable input so you can set this value. Then, display as range subtracts/adds 50 to whatever value you set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes more sense to have AVERAGE_READING_RATE be the constant and the range be a static +/- value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left an in-depth comment about the direction I think we should go on #53776 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is testing well for me 🎉 I also think this is a nice way to display the reading time.
- Displays a range of times âś…
- For short posts, the max time is never 1 âś…
- For existing blocks, still displays a single number âś…
If we want even more flexibility here, we could also add more display options from the other ideas in #71583 in further follow-ups.
useEffect( () => { | ||
if ( blockWasJustInserted ) { | ||
setAttributes( { | ||
displayAsRange: true, | ||
} ); | ||
} | ||
}, [ blockWasJustInserted ] ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I understand what this effect wants to do.
So you want to enable displayAsRange
only in newly inserted blocks? Do we need to enable displayAsRange
by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you want to enable displayAsRange only in newly inserted blocks?
Yes, because there will already be instances of this block in the wild that we don't want to change.
Do we need to enable displayAsRange by default?
This would change all the blocks already out there...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to set displayAsRange
to false in the newly inserted block as well? Do you think this is a problem from an A11y perspective?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once the block is inserted you can still turn the range off. I think this presents a good compromise - it encourages people to use the more accessible version but doesn't force it for those who really want to use the simpler version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, then, how about this approach:
- Define
displayAsRange
astrue
by default in block.json - Update
displayAsRange
tofalse
if theblockWasJustInserted
isfalse
Because I think the default values ​​in block.json should represent the actual behavior of the most recent block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this originally. The problem with this is the frontend rendering - if blocks aren't updated in the editor then they won't have displayAsRange
as false
so they will get converted to the new range format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the detailed explanation. I understand your approach is the best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this could have been done as part of the block deprecation migrate
method for old blocks.
Current logic seems inconsistent, IMO. Newly inserted blocks will have different defaults, while block.json
spec declares a different one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at doing this with a migration. The problem is that default attributes are not set, so a newly inserted block with displayAsRange: true
won't have any attribute set, which makes it indistinguishable from an old block which just doesn't have the attribute yet.
If you can find a way then i'm very open to it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've given this another test after all the recent changes, and it's working as expected:
- I can see a range of times when "Display as range" is enabled
- Existing blocks continue to display a single number
- The max time is never 1 for short posts
post_time_to_read_average_reading_speed
filter correctly updates the reading times
LGTM!
Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
…time calculations
2325b4d
to
97cea68
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update! I also tested it in the Japanese locale, and everything seems to work fine.
Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you may need to run npm run fixtures:regenerate
again, and make the PHP linter happy đź‘€
What?
Adds:
Closes #71583
Why?
Reading speed is different for different languages and individuals. By specifying a range we can be more inclusive of different languages and reading speeds.
How?
Add an option which allows the block to show a range of reading times, rather than one time.
Testing Instructions
Test filtering the average reading speed
Add this to the index.php file:
Check that reading times adjust accordingly.
Screenshots or screencast