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

Chore/update storybook etc #963

Merged
6 commits merged into from
Dec 18, 2018
Merged

Chore/update storybook etc #963

6 commits merged into from
Dec 18, 2018

Conversation

ghost
Copy link

@ghost ghost commented Dec 14, 2018

PR Checklist

  • Manually tested across supported browsers
    • Chrome
    • Firefox
    • Safari
    • Edge (Win 10)
  • Unit tests written (common at minimum)
  • PR has one of the semver- labels
  • Two core team engineer approvals
  • One core team UX approval

});
}
const portalElement = window.document.createElement('div');
window.parent.document.body.appendChild(portalElement);
Copy link
Author

Choose a reason for hiding this comment

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

storybook puts content in an iframe, so the statically positioned toggle would be pinned to the iframe. We shouldn't run into any cross-origin issues.

showAddonPanel: !urlState.addons,
});
}
export const withPanelToggles = makeDecorator({
Copy link
Author

Choose a reason for hiding this comment

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

Using storybook's decorator functionality since we only want the toggle visible on "example" pages and not our "article" pages.

// For some reason this doesn't pickup our .babelrc even though it's supposed to
return babelCore.transform(codeMap[specifierType], { presets: ['es2015'] })
.code;
return babelCore.transform(codeMap[specifierType]).code;
Copy link
Author

Choose a reason for hiding this comment

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

babel presets were removed. Seems to pick up babelrc fine now.

@@ -284,7 +284,7 @@ describe('Autocomplete', () => {
});

afterEach(() => {
if (wrapper) {
if (wrapper && wrapper.exists()) {
Copy link
Author

Choose a reason for hiding this comment

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

fixes some flaky tests where we were trying to unmount components already unmounted.

@@ -7,7 +7,9 @@ exports[`Collapsible [common] example testing should match snapshot(s) for 1.tog
"height": 0,
}
}
/>
>
Copy link
Author

Choose a reason for hiding this comment

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

enzyme shallow no longer uses a self closing tags, and now includes the immediate child's text/node

render() {
const urlState = this.props.api.getUrlState();

if (urlState.full || !urlState.addons) {
Copy link
Author

Choose a reason for hiding this comment

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

urlState.addons intended use is to control the visibility of the panel sidebar. Using that here creates a bug where if the user hides the sidebar and later reloads the url we don't show the toggle component. Example: https://docspot.adnxs.net/projects/lucid/latest/?selectedKind=DropMenu&selectedStory=stateless&full=0&addons=0&stories=1&panelRight=1&addonPanel=lucid-docs-panel-props

This code is effectively replaced by using the new panelToggle decorator.

@ghost
Copy link
Author

ghost commented Dec 14, 2018

There is a regression in this PR where we no longer remember the visibility or selection state of the side panel when navigating between stories. We need to set options on each individual story so that the Article stories hide the sidebar, and Example stories show the sidebar. Storybook does all of this while initializing, and I can't see a way to merge these overrides with user's current selections. I tried a couple of different approaches, but couldn't get it working the way I wanted it to. The current approach is at least consistent where example pages show the sidebar on the right and article pages hide the sidebar.

@jondlm
Copy link
Contributor

jondlm commented Dec 14, 2018

Seems like a minor enough issue to me. I'd much rather have all these upgrades!

package.json Outdated
@@ -2,6 +2,9 @@
"name": "lucid-ui",
"version": "2.36.5",
"description": "A UI component library from AppNexus.",
"engines": {
"node": ">=8.12.0"
Copy link
Author

Choose a reason for hiding this comment

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

I think I need to remove this since it throws an error during npm install if the node engine doesn't match. Is there a better way to flag that development must be done >=8, but it doesn't matter for consumers of the library?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good question. I've seen folks use a little bash script to check the version of node instead of engines but it's certainly not as clean. As far as I can tell there isn't a clear hook we could use to only run a script when running yarn install from within lucid (i.e. you're a developer working on lucid). Maybe we should just skip this problem for now and instead document it in the README?

@codecov-io
Copy link

Codecov Report

Merging #963 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #963      +/-   ##
==========================================
- Coverage   90.06%   90.04%   -0.03%     
==========================================
  Files         183      183              
  Lines        3453     3456       +3     
  Branches     1110     1112       +2     
==========================================
+ Hits         3110     3112       +2     
+ Misses        322      321       -1     
- Partials       21       23       +2
Impacted Files Coverage Δ
src/components/Icon/Icon.jsx 81.25% <ø> (ø) ⬆️
.../TextFieldValidated/TextFieldValidated.reducers.js 0% <0%> (ø) ⬆️
src/components/SidePanel/SidePanel.jsx 74.46% <0%> (+4.25%) ⬆️

@ghost ghost merged commit a312c66 into master Dec 18, 2018
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants