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 Storybook 2.0 #500

Merged
merged 7 commits into from
Sep 12, 2019
Merged

UI Storybook 2.0 #500

merged 7 commits into from
Sep 12, 2019

Conversation

drwpow
Copy link
Contributor

@drwpow drwpow commented Sep 12, 2019

Reason for change

This PR originally started with just using Knobs to their fullest extent, but ended up combining all our stories into just a few for easier testing.

Screen Shot 2019-09-12 at 08 56 30

The old way

  • 1 story collection per component, resulting in dozens of stories that became overwhelming
  • Components that were meant to be used together, weren’t
  • We didn’t know about Storybook Knobs, so testing resources were frustrating (I guess I don’t have a resource named “my-resource” so I can’t test this…)

This PR

  • Stories are now grouped into 4 categories:
    • Common: buttons, tags, toasts, and other basic building blocks
    • Catalog: listing, browsing, and displaying products are all grouped together
    • Resource: resource operations are combined into only a couple views, similar to Render
    • Scenarios: one-off scenarios to be tested, such as the auth iframe or metrics logging
  • Related components are combined into one story so they can interact with one another
  • Knobs are used carefully, so that components are flexible (but without too many knobs where you have to start doing work to use them).
  • Now we can set manifold_api_token with a text input! 🎉 And it sticks using localStorage

Testing

Play around with the Storybook stories, and any feedback there is welcome.

Checklist

  • CHANGELOG: The Unreleased section of CHANGELOG was updated
  • Prop changes: docs were updated
  • Prop changes: E2E tests were updated, testing from the highest level possible
  • Platform testing: If this change should be tested against a platform, has it been?

@vercel
Copy link

vercel bot commented Sep 12, 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://ui-git-dangodev-more-knobs.manifold.now.sh

@drwpow drwpow temporarily deployed to manifold-ui-stage-pr-500 September 12, 2019 12:49 Inactive
@vercel vercel bot temporarily deployed to staging September 12, 2019 12:52 Inactive
@drwpow drwpow temporarily deployed to manifold-ui-stage-pr-500 September 12, 2019 12:53 Inactive
@drwpow drwpow force-pushed the dangodev/more-knobs branch from 801fdf9 to 40914a4 Compare September 12, 2019 12:54
@codecov
Copy link

codecov bot commented Sep 12, 2019

Codecov Report

Merging #500 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #500   +/-   ##
=======================================
  Coverage   78.99%   78.99%           
=======================================
  Files          51       51           
  Lines        1738     1738           
  Branches      460      460           
=======================================
  Hits         1373     1373           
  Misses        349      349           
  Partials       16       16

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 e631ae3...286086f. Read the comment docs.

@drwpow drwpow temporarily deployed to manifold-ui-stage-pr-500 September 12, 2019 12:54 Inactive
@drwpow drwpow temporarily deployed to manifold-ui-stage-pr-500 September 12, 2019 12:59 Inactive
},
false
);
</script>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the Storybook-recommended approach to add event listeners when the page is loaded, or stories are changed

font-size: 0.8735804647em;
font-weight: 700;
padding: 0.5em 1.25em;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adds some basic styling to buttons in stories

The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project
adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
Copy link
Contributor Author

@drwpow drwpow Sep 12, 2019

Choose a reason for hiding this comment

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

This, and pretty much everything other than the 1 CHANGELOG addition are all Prettier autoformatting changes. This is because the 2 Prettier configs were combined, and this is the first time proseWrap has been run on this CHANGELOG.

// features: {
// // …
// },
// }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Minor doc change

@@ -80,7 +80,7 @@
&[data-color='pink'] {
color: var(--manifold-grayscale-100i);
background: var(--manifold-g-pink);
border-color: transparent;
border-style: none;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In testing out the stories, I realized buttons had a border they shouldn’t have. This fixes it.

// eslint-disable-next-line prefer-destructuring
this.resource = response[0];
this.resource = Array.isArray(resource) ? resource[0] : resource;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fixes a bug I found where Gateway can return individual resources not in an array.

Choose a reason for hiding this comment

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

Gateway should always return a single resource when using the /resources/me/:label endpoint, https://github.com/manifoldco/gateway/blob/master/specs/v1.yaml#L203

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gateway should always return a single resource when using the /resources/me/:label endpoint, https://github.com/manifoldco/gateway/blob/master/specs/v1.yaml#L203

@Minivera maybe this is a bug in Gateway? I was definitely seeing no array for one of my resources. I thought the same, but this was necessary for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just tried again. It’s definitely an object for me. Maybe for Gateway this is an object; and it was only an array on Marketplace?

Choose a reason for hiding this comment

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

Yep, gateway returns a single object and marketplace returns an array with on object when filtered with a label.

Choose a reason for hiding this comment

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

We should probably update the returned types, it should not be an array according to swagger.

quicker. However, we do use a knob for color.). Too many knobs is bad, as is too few.
- Organize by user flow. Try to recreate common workflows users would have.
- Need to place JavaScript on a story page? Place it in `./storybook/preview-head.html` (`<script>`
tags won’t run like HTML will)
Copy link
Contributor Author

@drwpow drwpow Sep 12, 2019

Choose a reason for hiding this comment

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

Added a new section on how to write good stories for Storybook. The rest of the changes are just proseWrap formatting changes from Prettier (the file hadn’t been edited since combining the two Prettier configs)

return `
<style>img { height: 256px; width: 256px; }</style>
<manifold-data-product-logo product-label="${productLabel}"></manifold-data-product-logo>`;
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Story collection 1: Catalog

>
${message}
</manifold-toast>`;
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Story collection 2: common UI elements (open to better naming)

Choose a reason for hiding this comment

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

Utility UI elements?

</manifold-data-deprovision-button>
</menu>
</manifold-resource-container>`;
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Story collection 3: resource-related stories.

For these, we save manifold_api_token to localStorage to persist across refreshes. We also save resource-label to localStorage so you can bounce around between stories while working on the same resources.

<p>Shortly after this component loads, you should see <a href="https://app.datadoghq.com/logs?cols=%22%5B%5C%22core_host%5C%22%2C%5C%22core_service%5C%22%5D%22&event&from_ts=1566584997801&index=main&live=true&messageDisplay=inline&query=%40browser-source%3Amanifold-ui&stream_sort=desc&to_ts=1566585897801">logs in DataDog</a></p>
<manifold-plan product-label="elegant-cms" plan-label="manifold-developer"></manifold-plan>
</manifold-performance>`
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Story collection 4: potpourri scenarios that don’t fit into a larger collection. Also open to naming here, too.

Copy link

@Minivera Minivera left a comment

Choose a reason for hiding this comment

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

Amazing work!

}

// listen to plan selector, update any/all buttons on the page
function updatePlanResize() {

Choose a reason for hiding this comment

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

You are giving jQuery flashbacks.

Joke aside, would it be possible to create code that only run on specific stories? I can see this file becoming big

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately not. Any <script> tags you add to stories don’t get executed because it basically inserts them as text / HTML nodes. This is the only way to do it in v5.

But this also lends itself to fewer stories as well—it encourages more components working together in fewer stories.

Choose a reason for hiding this comment

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

not sure if I understand correctly, but is the problem that you want to have a script tag inside a string in a script tag like this?

<script>
  /* ... something something */
  const js = `
    <script></script>
  `
</script>

If this is the problem you can escape the end tag like:

<script>
  /* ... something something */
  const js = `<script><\/script>`
  // or...
  const js = `<script></${'script'}>`
</script>

Copy link
Contributor Author

@drwpow drwpow Sep 12, 2019

Choose a reason for hiding this comment

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

Not exactly; it’s just weird. It inserts the <script> tag as you’d expect—it makes it on the page—but I wasn’t getting it to execute, and other people report the same. See storybookjs/storybook#6113.

// eslint-disable-next-line prefer-destructuring
this.resource = response[0];
this.resource = Array.isArray(resource) ? resource[0] : resource;

Choose a reason for hiding this comment

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

Gateway should always return a single resource when using the /resources/me/:label endpoint, https://github.com/manifoldco/gateway/blob/master/specs/v1.yaml#L203

>
${message}
</manifold-toast>`;
});

Choose a reason for hiding this comment

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

Utility UI elements?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants