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

Editor: don't alter browser history with page persistence #5321

Merged
merged 2 commits into from
Nov 20, 2020

Conversation

maxyinger
Copy link
Contributor

Summary

Makes hash updates not effect browser history.

Relevant Technical Choices

Moved to using history.replaceState(...) vs editing the hash directly. This keeps the app from altering history when editing persistent hash state.

To-do

NA

User-facing changes

Now when you change pages in the editor, then press the back button in the browser, you should be taken back to the last page you were on before the editor

Testing Instructions

Go to my stories -> open a story in the editor -> click to view several different pages in your story or add a page -> press the browser's back button once -> should go directly back to the stories view.


Fixes #4858

@google-cla google-cla bot added the cla: yes label Nov 16, 2020
@maxyinger maxyinger changed the title 4858 history and hash Edit Story: don't alter history with page persistence Nov 16, 2020
@github-actions
Copy link
Contributor

Size Change: +34 B (0%)

Total Size: 1.44 MB

ℹ️ View Unchanged
Filename Size Change
assets/css/edit-story.css 909 B 0 B
assets/css/stories-dashboard.css 939 B 0 B
assets/css/web-stories-embed-block.css 515 B 0 B
assets/js/chunk-fonts-********************.js 44 kB 0 B
assets/js/chunk-web-stories-template-0-********************.js 10.7 kB 0 B
assets/js/chunk-web-stories-template-1-********************.js 11.4 kB 0 B
assets/js/chunk-web-stories-template-2-********************.js 11.1 kB 0 B
assets/js/chunk-web-stories-template-3-********************.js 11.2 kB 0 B
assets/js/chunk-web-stories-template-4-********************.js 12.6 kB 0 B
assets/js/chunk-web-stories-template-5-********************.js 7.08 kB 0 B
assets/js/chunk-web-stories-template-6-********************.js 10.2 kB 0 B
assets/js/chunk-web-stories-template-7-********************.js 10.3 kB 0 B
assets/js/chunk-web-stories-textset-0-********************.js 5.25 kB 0 B
assets/js/chunk-web-stories-textset-1-********************.js 6.58 kB 0 B
assets/js/chunk-web-stories-textset-2-********************.js 7.83 kB 0 B
assets/js/chunk-web-stories-textset-3-********************.js 15.2 kB 0 B
assets/js/chunk-web-stories-textset-4-********************.js 4.38 kB 0 B
assets/js/chunk-web-stories-textset-5-********************.js 5.68 kB 0 B
assets/js/chunk-web-stories-textset-6-********************.js 5.47 kB 0 B
assets/js/chunk-web-stories-textset-7-********************.js 10.3 kB 0 B
assets/js/edit-story.js 547 kB +14 B (0%)
assets/js/stories-dashboard.js 613 kB +20 B (0%)
assets/js/web-stories-activation-notice.js 74 kB 0 B
assets/js/web-stories-embed-block.js 17.2 kB 0 B

compressed-size-action

@swissspidy
Copy link
Collaborator

Thank you!

@codecov
Copy link

codecov bot commented Nov 16, 2020

Codecov Report

Merging #5321 (0ce5895) into main (0e19536) will increase coverage by 0.13%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5321      +/-   ##
==========================================
+ Coverage   76.37%   76.50%   +0.13%     
==========================================
  Files         962      962              
  Lines       16886    16886              
==========================================
+ Hits        12896    12919      +23     
+ Misses       3990     3967      -23     
Flag Coverage Δ
karmatests 53.88% <100.00%> (+1.32%) ⬆️
unittests 66.36% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...s/src/edit-story/app/story/effects/useHashState.js 100.00% <100.00%> (ø)
assets/src/edit-story/components/keyboard/index.js 82.35% <0.00%> (+1.17%) ⬆️
...dit-story/components/fontPicker/pickerContainer.js 100.00% <0.00%> (+5.88%) ⬆️
...edit-story/components/fontPicker/fontList/index.js 96.72% <0.00%> (+8.19%) ⬆️
assets/src/edit-story/utils/keyboardOnlyOutline.js 95.65% <0.00%> (+8.69%) ⬆️
...dit-story/components/fontPicker/fontSearchInput.js 92.00% <0.00%> (+12.00%) ⬆️
assets/src/edit-story/app/font/fontProvider.js 64.00% <0.00%> (+22.00%) ⬆️

Copy link
Contributor

@carloskelly13 carloskelly13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good and keeps the correct page on navigation, I can't think of an offhand reason this would break anything, but would be great for @swissspidy and @barklund to give it a spot check.

Copy link
Contributor

@BrittanyIRL BrittanyIRL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@swissspidy swissspidy added Type: Bug Something isn't working Group: Workspace A group encompassing all of workspace – everything inside the canvas and sidebar Pod: Pea labels Nov 20, 2020
@swissspidy swissspidy merged commit d3b159f into main Nov 20, 2020
@swissspidy swissspidy deleted the 4858-history-and-hash branch November 20, 2020 14:27
divyarajmasani pushed a commit to rtCamp/web-stories-wp that referenced this pull request Nov 26, 2020
@swissspidy swissspidy changed the title Edit Story: don't alter history with page persistence Editor: don't alter history with page persistence Dec 10, 2020
@swissspidy swissspidy changed the title Editor: don't alter history with page persistence Editor: don't alter browser history with page persistence Dec 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Group: Workspace A group encompassing all of workspace – everything inside the canvas and sidebar Type: Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Workspace: Page hash causes issues with browser history
5 participants