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

Docs2: Refactor manager to use new index data #18023

Merged
merged 28 commits into from
May 9, 2022

Conversation

tmeasday
Copy link
Member

@tmeasday tmeasday commented Apr 22, 2022

Previously,

We massaged data in three places:

  1. When receiving a set of stories either from SET_STORIES (v6) or stories.json (v7), we denormalized it (in api/lib/stories.ts) by creating entries in the storiesHash for each root, group and component that contained a story.
    i. We represented "docs nodes" in this representation as stories with parameters.docsOnly = true
    ii. As the story index didn't contain parameters, we did a hack/heuristic to check if a story was the only child of a component and had the name Page, and prefilled parameters.docsOnly if so.

  2. When rendering the sidebar (in ui/components/data.ts), we "collapse" stories into their components in the following ways:
    i. For normal mode, we'd pick out docs only stories (based on the parameters.docsOnly) and collapse them into their containing component, ending up with an entry in the hash with isComponent = isLeaf = true and a story id.

ii. For DOCS_MODE, we'd collapse the first story of every component into it, as above, and delete all the other stories.

  1. When rendering the tree (ui/components/sidebar/Tree.tsx), we'd:
    ii. Move "unrooted" components and groups to the front of the hash
    ii. Collapse the data another time to deal with "single story components" -- components with a single contained story that has the same name as the component. collapse the data another time to deal with "single story components" -- components with a single contained story that has the same name as the component.

My thinking in this PR is that there are two problems with the above:

A. We no longer represent docs entries in the index as parameters.docsOnly stories, so we can get away with doing the same in the storiesHash and make a much clearer distinction between story entries (which can be rendered in both view modes, currently) and docs entries (which can only be rendered in docs view mode).

B. The reason we denormalize into storiesHash (and don't pass around the StoryIndex data structure) is to make our life easier when doing things like goToNextComponent etc. However that breaks down if the storiesHash doesn't actually represent what we are rendering in the sidebar (which is what happens when doing later collapsing in 2&3).

For instance, you cannot sensibly navigate to "unrooted" stories with the keyboard shortcuts, because the position they appear in the list is different to the internal representation.

So the solution appears to be to do all the "massaging" initially when creating the storiesHash


Considering "Single Story Components":

There are three cases were we want to combine or collapse a component and its child(ren):

  • When there is one child story, and the name of the child is the same as the component
  • When there is one child, and the child is a docs entry
  • In all cases when in DOCS_MODE.

When we collapse a component into its children, we want to make the component:

  • Keep the name of the component.
  • Display the icon of the child.
  • "Act" as the child, ie. emit the right message when you click on it.

We have a choice here, we could either component the ComponentEntry and StoryEntry (or DocsEntry) into a single entry (of a new type), or we could simply render the two entries as a single node in the tree.

To decide, we should think carefully about how we want keyboard navigation to work. Suppose we have three stories:

a: Components / Button : Primary
b: Components / Graph : Graph
c: Components / Table : Empty

The second component is a "single story" component (the other two are not as they have different names from their only story). How should keyboard navigation work?

  • When on a, "next story" should take you to b, "next component" should take you to b also
  • When on b, "next story" should take you to c, "next component" should take you to c also

In this way, we want to act on the has as if the original structure was there.


In this PR,

  • The types for the HashEntrys (in the StoryHash) are simplified via the use of a type field and separated cleanly into RootEntry, GroupEntry, ComponentEntry, DocsEntry and StoryEntry.

  • When rendering in normal mode, a component entry with a single docs entry is created for docs pages.

  • When creating the storiesHash, the unrooted stories are sorted to the front automatically.

  • When rendering in DOCS_MODE, we filter out all non-first children when creating the hash (1 above), and rename the first child to the name of the component. -- I decided to this in a future PR, as it wasn't strictly necessary for the other changes, just related.

  • When rendering the tree, we render the combination of a ComponentEntry and its "single story" (see above) as a single StoryNode or DocsNode.

@nx-cloud
Copy link

nx-cloud bot commented Apr 22, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 349ba7b. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@shilman shilman added ui maintenance User-facing maintenance tasks labels Apr 22, 2022
lib/api/src/lib/stories.ts Show resolved Hide resolved
lib/api/src/lib/stories.ts Show resolved Hide resolved
lib/api/src/lib/stories.ts Show resolved Hide resolved
lib/api/src/lib/stories.ts Outdated Show resolved Hide resolved
lib/api/src/modules/stories.ts Outdated Show resolved Hide resolved
@tmeasday tmeasday marked this pull request as ready for review April 23, 2022 02:49
@shilman shilman marked this pull request as draft April 28, 2022 02:28
@tmeasday tmeasday marked this pull request as ready for review April 28, 2022 12:06
lib/ui/src/components/sidebar/Tree.tsx Outdated Show resolved Hide resolved
lib/ui/src/components/sidebar/__tests__/data.test.ts Outdated Show resolved Hide resolved
});
});

it('collapses mixtures of leaf and non-leaf children', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

This tested an impossible situation -- you cannot have a story as a child of a root. Perhaps there was something legitimate this was trying to test, I am not sure. Aiming to revisit DOCS_MODE later.

tmeasday added a commit that referenced this pull request May 2, 2022
For forwards compatibility with changes coming in 7.0, see: #18023
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.

@tmeasday looks like there are a bunch of unintentional changes in chromatic. can you please take a look? 🙏

@tmeasday
Copy link
Member Author

tmeasday commented May 9, 2022

@shilman the 3 remaining Chromatic changes exist on the base branch future-hybrid-stories.

lib/api/src/lib/stories.ts Outdated Show resolved Hide resolved
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.

Amazing job @tmeasday !!! Merging 🎉

@shilman shilman changed the title Refactor manager to use new index data Docs2: Refactor manager to use new index data May 9, 2022
@shilman shilman merged commit cf0529e into future-hybrid-stories-index May 9, 2022
@shilman shilman deleted the future/docs2/manager-ui branch May 9, 2022 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance User-facing maintenance tasks ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants