-
Notifications
You must be signed in to change notification settings - Fork 132
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
Keep save button text updated, without errors. #585
Conversation
Couple of extra notes:
|
@WPprodigy Looks good! I thought about using a MutationObserver originally, but was waffling on IE support. I think it's probably fine. If we're going to overhaul it this much I'd like to roll in these changes that support setting the correct label and these e2e tests that validate it. If you can add those changes along with the e2e tests I'd feel confident enough to merge the change. |
Also, add a |
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 good to me— I tried testing a bunch of different scenarios: different post statuses, changing the status, publishing & unpublishing… I also did a quick test with GB 7.1, and no crashing 🎉
It might be worth adding a note to your function comment with the background for why this is a workaround, and what potential solutions are/what they're waiting on.
@cojennin Could we do the eslint / standards fixing in a followup PR right after this? And then after that, a new PR for the feature of adding the correct labels into the button can follow. I see a few PRs now have tried the above, would prefer to do these in piece mail so they stop getting left behind :). That way we can get this out the door, and it's more visible what happened when. And then agreed, I'll get some comments added, perhaps just linking back to the issue for now to have the context and make it easier to remove in the future. |
@WPprodigy Sure, let's move forward on merging this PR. For future PR's I'd like to get in the habit of incorporating e2e tests where possible for frontend changes, just to make sure we're not regressing (as @mjangda cautioned in #555, want to avoid things like infinite loops or exponentially increasing observers) |
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.
Should be good to go (I updated the branch through the Github UI, just make sure that didn't cause any issues then feel free to merge)
Fixes #583. A different approach to #584.
Fixing the page-breaking error
On the initial load, when the
subscribe()
method triggers we can update the button text right away from when it first exists. By checking specifically if theinnerText
is one of the two values we don't want, we avoid the errors that occur in #583.Keeping the button text changes even after post saving
Then there is the issue of the button text reverting back to "Save draft" / "Save as Pending" after saving the post.
When a post is being saved, the DOM change quite a bit (4 different states) and there is unfortunately no way that I could find to hook in after the save is completed and the button is back to it's default state (the last animation tick being particularly annoying). To workaround this limitation, I'm using the MutationObserver API if the browser has it available: https://developer.mozilla.org/en-US/docs/Web/API/MutationObserver
So then when the
subscribe()
method is triggered and the post is being saved, we set up a single observer on the parent element. This observer waits for the button to be added back then it targets theinnerText
again and get it updated. This node is actually re-added quite a few times per post-save, and due to various animations it's not actually possible to set/remove the observer each time. So instead, I decided to just set up one observer and keep it running. It watches over the minimal amount of nodes necessary.Testing