Dashboard: Add global Page Advancement setting #12460
Conversation
miina
added
Type: Enhancement
packages/wp-dashboard/src/components/editorSettings/pageAdvancement/index.js
Outdated
Show resolved
Hide resolved
| defaultPageDuration: 7, | ||
| ...story, | ||
| }; | ||
| return story as StoryV11; |
There was a problem hiding this comment.
Have to remove always setting the auto-advance and page duration, otherwise we can't really detect if it needs to be overridden by global settings.
There was a problem hiding this comment.
@merapi FYI I stepped through the code locally and looks like the version is undefined for a new story, meaning that every new story will go through the full migration script initially 🤷 Not sure if that's the intended way or a bug.
There was a problem hiding this comment.
@swissspidy Perhaps you have thoughts about this?
Looks like new stories will always go through all migrations. Since V11 adds auto-advance props but we shouldn't have those by default on new stories to use the global ones instead, we don't need V11 migration anymore. Should we just remove it? Is it a bug that the migration runs through new stories?
There was a problem hiding this comment.
Looks like new stories will always go through all migrations. Is it a bug that the migration runs through new stories?
AFAIK it's intentional because this way a new story automatically gets the right and up-to-date data structure.
We shouldn't modify existing migrations though, since this won't work for existing stories.
It'd be better to add a new migration, e.g. to delete defaultPageDuration if it's 7.
There was a problem hiding this comment.
It'd be better to add a new migration, e.g. to delete defaultPageDuration if it's 7.
Yep, I wasn't sure about what's the best way to proceed with that -- removed the current migration to make it work functionally.
If we add a new migration, the existing stories with the default values would be changed and this would mean the new global values would influence those stories, too, not just the new stories. If that's ok then this sounds like a good plan.
There was a problem hiding this comment.
The new global value will default to the same number as before, right? So a story would still behave the same.
There was a problem hiding this comment.
Not necessarily. The new global value will be default indeed, however, if the user first changes the global value, and then opens an existing story with previous default values, it will use the new global values for the story -- since the migration runs only if a story is opened, and it can be opened after the global value has been changed.
|
Plugin builds for cd20bc2 are ready 🛎️!
|
|
Size Change: +1.28 kB (0%) Total Size: 2.72 MB
ℹ️ View Unchanged
|
|
Couple of questions and I think we need to work out what is going on with the migration, otherwise this is looking good to go. |
Yep, that's still a todo in the description, too :) Pascal also recommended adding a new migration. If it's ok that all the previous stories with default values be influenced as well by the new global values, then sounds like a good plan. |
|
I can confirm that changing the value of auto page advancement time is saved. When I create a new story, the global value is applied to the new story only (does not affect previously created stories). However, when I publish the story and look at the source, it looks like the updated value is NOT passed on to the amp Example:
Test Story URL: https://stories-qa-wordpress-amp.pantheonsite.io/wp-admin/post.php?post=20421&action=edit#page=%2522e538f99e-de42-4385-987d-d02a3d867645%2522
|
|
I feel this ticket needs some more work.
|
Good catch! I saved the story before publishing so the values were correct in the front-end, too. Should be fixed now without saving, too.
Fixed, thanks! |
|
@miina -- now the global value for page adv is applied by directly to the new story and within the amp (both draft and published version) as expected. I also tested other ways of creating a story (duplicate existing story, New > Story from top menu, Stories > Add new) and the page adv value matches the expected. However, when I create a web-story from template, the updated global value is NOT applied Test:
Expected: the new global age adv 15 sec or manual I think the path to create a new story from template should honor the global page adv settings. Cc: @swissspidy |
|
@swissspidy @miina - I think this ticket should be in-progress since creating the story via templates does not pick up the global page adv settings. So I am going to move this ticket back so that it reflects the current status. |
|
Good catch @kkalarickal but I think we should proceed with what we have here and create a follow-up to solve the template case because for me it's not so obvious that is should be taken from the global settings and not from the template settings. Is everything else fine here? |
@merapi Are you referring to custom saved page templates? Or the story templates from the dashboard? |
Templates from the dashboard. So the question is - whether the template creator would like the new story pages to have a specific auto-advance rather than the global one. Is there a use case for that? |
Yes @merapi -- the global auto advance settings are working fine on all other paths for creating a new story. If the templates from dashboard have their own auto adv setting that is not over-riden by the global setting, then we should probably mention that in the Default Page Advancement blurb "All new stories will be set to this page advancement setting, except when created from default templates...." |
💯
My biggest issue with this is: |
|
For visibility: it was decided that we'll use global value for the templates. Will create a follow-up PR for that. |
Context
Adds Page Advancement global option to the Dashboard settings which will be used if the setting hasn't been set per story.
Summary
Relevant Technical Choices
To-do
User-facing changes
Testing Instructions
This PR can be tested by following these steps:
Reviews
Does this PR have a security-related impact?
Does this PR change what data or activity we track or use?
Two new tracking values:
change_auto_advanceandchange_default_page_durationDoes this PR have a legal-related impact?
Checklist
Type: XYZlabel to the PRFixes #11528