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

Fix browser navigation #2261

Merged
merged 2 commits into from
Nov 8, 2017
Merged

Fix browser navigation #2261

merged 2 commits into from
Nov 8, 2017

Conversation

Hypnosphi
Copy link
Member

@Hypnosphi Hypnosphi commented Nov 8, 2017

Issue: #2134
Currently we spoil browser history with tons of entries, especially when using addon-knobs

What I did

Used replaceState most of the times, which doesn't create a new history entry, but replaces the current URL and state object. pushState is only used when user goes to a new story (each story may be considered a "page")

How to test

Open cra-kitchen-sink, navigate to some story with knobs, hit "back" button

@codecov
Copy link

codecov bot commented Nov 8, 2017

Codecov Report

Merging #2261 into master will decrease coverage by 0.04%.
The diff coverage is 8.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2261      +/-   ##
==========================================
- Coverage    22.2%   22.16%   -0.05%     
==========================================
  Files         268      268              
  Lines        5872     5883      +11     
  Branches      712      709       -3     
==========================================
  Hits         1304     1304              
- Misses       4023     4054      +31     
+ Partials      545      525      -20
Impacted Files Coverage Δ
lib/ui/src/modules/ui/configs/handle_routing.js 24.73% <8.33%> (-3.32%) ⬇️
lib/ui/src/modules/ui/libs/filters.js 47.36% <0%> (ø) ⬆️
lib/ui/src/modules/ui/components/layout/usplit.js 38.7% <0%> (ø) ⬆️
lib/codemod/src/transforms/update-addon-info.js 50% <0%> (ø) ⬆️
...rc/modules/ui/components/left_panel/text_filter.js 30.98% <0%> (ø) ⬆️
lib/ui/src/modules/shortcuts/actions/shortcuts.js 6.25% <0%> (ø) ⬆️
addons/knobs/src/KnobStore.js 9.09% <0%> (ø) ⬆️
addons/knobs/src/components/types/Object.js 5.88% <0%> (ø) ⬆️
addons/knobs/src/components/PropForm.js 9.09% <0%> (ø) ⬆️
... and 20 more

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 5627316...b7c9ace. Read the comment docs.

const { selectedKind, selectedStory } = clientStore.getAll();
// use pushState only when a new story is selected
const usePush =
prevKind != null &&
Copy link
Member

Choose a reason for hiding this comment

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

Is there something to push the state for the first story storybook loads when there isn't a previous kind? Or is that not really necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

Imagine that we opened storybook by a link from other website. If we'll create additional history entries on initialization stage, we won't be able to return to referring website by just hitting "back" button.

That's why we need to replace not push in this case

Copy link
Member

Choose a reason for hiding this comment

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

gotcha, makes sense

const { selectedKind, selectedStory } = clientStore.getAll();
// use pushState only when a new story is selected
const usePush =
prevKind != null &&
Copy link
Member

Choose a reason for hiding this comment

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

gotcha, makes sense

@Hypnosphi Hypnosphi merged commit fdbde92 into master Nov 8, 2017
@Hypnosphi Hypnosphi deleted the replace-state branch November 8, 2017 10:17
@Hypnosphi Hypnosphi changed the title Use replaceState instead of pushState when the story stays the same Fix browser navigation Nov 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants