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

UI: Fix settings page route (about, shortcuts) #7241

Merged
merged 14 commits into from
Jul 10, 2019

Conversation

lonyele
Copy link
Member

@lonyele lonyele commented Jul 1, 2019

Issue: #6741

Summary

Make settings page route properly by changing viewMode state so that only settings page is rendered.

What I did

  • Change viewMode state to settings by introducing new api showSettingsPage when it is routed to settings pages(about, shortcuts, and more in the future)
  • Persist settings page URL after refreshing the browser by adding fallback logic to setStories
  • Add settings to PropTypes temporarily for making console warnings silent(see TODO section below)

More context

Problem

As this was implemented, viewMode has to be changed to settings for rendering only settings page component.

What is missing here(TODO)

I think the following PR is needed to resolve this issue of PropType warnings by getting the lists of possible viewMode from all the registered addons and others for strongly type viewMode.

@vercel
Copy link

vercel bot commented Jul 1, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://monorepo-git-fork-lonyele-fix-settings-page-route.storybook.now.sh

@lonyele lonyele added the bug label Jul 1, 2019
@lonyele
Copy link
Member Author

lonyele commented Jul 1, 2019

Hm... I merged lastest next branch to fix the source-loader.test.js fail. Maybe I messed up something here, because CircleCI is not running...

@lonyele
Copy link
Member Author

lonyele commented Jul 1, 2019

or... it went fine. Now CI looks fine and chromatic also looks fine. Would anyone review this one?

@lonyele
Copy link
Member Author

lonyele commented Jul 2, 2019

oh wait... I'll write a test for this

@shilman
Copy link
Member

shilman commented Jul 9, 2019

@lonyele I'm not thrilled with the idea of adding a new API for this. In general it's better to keep API surface area smaller unless it's needed. That said, the code all looks pretty good to me. @ndelangen what do you think?

@shilman shilman added the ui label Jul 9, 2019
@shilman shilman added this to the 5.2.0 milestone Jul 9, 2019
@lonyele
Copy link
Member Author

lonyele commented Jul 9, 2019

@shilman Yeah... I really didn't want to use setState way instead of just using navigate(...) because all the code was implemented that way. I couldn't find the real cause of this problem.

@lonyele
Copy link
Member Author

lonyele commented Jul 9, 2019

OH.... wait I found it.

@lonyele
Copy link
Member Author

lonyele commented Jul 9, 2019

Yesss! I've been having this feeling that I almost found it but couldn't really confirm it. What about now!

@@ -82,14 +82,14 @@ const createMenu = memoize(1)((api, shortcutKeys, isFullscreen, showPanel, showN
{
id: 'about',
title: 'About your Storybook',
onClick: () => api.showSettingsPage('about'),
onClick: () => api.navigate('/settings/about'),
Copy link
Member

Choose a reason for hiding this comment

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

@lonyele Doesn't this change fail to change the viewMode?

Copy link
Member Author

@lonyele lonyele Jul 10, 2019

Choose a reason for hiding this comment

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

No! I made the new api because I wasn't sure the place where assigning value to viewMode. However I confirmed that parsePath is the very first place where assigning value to viewMode

The reason why this bug happened is because settings viewMode was excluded from assigning value thus always return undefined which makes viewMode at the component level always become a story. At the end, url is /settings/about but viewMode is story thus showing both story preview component and settings page component.

const knownNonViewModesRegex = /(settings)/;
export const parsePath: (path?: string) => StoryData = memoize(1000)(
  (path: string | undefined | null) => {
    const result: StoryData = {
      viewMode: undefined,
      storyId: undefined,
    };

    if (path) {
      const [, viewMode, storyId] = path.match(splitPathRegex) || [undefined, undefined, undefined];
      if (viewMode && !viewMode.match(knownNonViewModesRegex)) {
        Object.assign(result, {
          viewMode,
          storyId,
        });
      }
    }
    return result;
  }
);

While I was trying to change different parts of the code, I got lost on the real cause of this problem. It's good that I finally found it! Please check now deployment!

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Awesome. Thanks for seeing this through!! Great fix 👏

@shilman shilman merged commit 6b577eb into storybookjs:next Jul 10, 2019
@shilman shilman changed the title Fix settings page route(about, shortcuts) UI: Fix settings page route (about, shortcuts) Jul 10, 2019
@lonyele lonyele deleted the fix/settings-page-route branch July 19, 2019 05:40
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