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

Viewport styles #25897

Open
wants to merge 7 commits into
base: next
Choose a base branch
from
Open

Conversation

jasonsperske
Copy link

What I did

Added a small bit of code to the addon/viewport that adds a data attribute to the iframe body element (containing #storybook-root) called data-storybook-viewport-id. This lets you define viewport specific styles in .storybook/preview.css such as

#storybook-root {
  padding: 12px;
}
body[data-storybook-viewport-id="mobile1"] #storybook-root {
  padding: 8px;
}
body[data-storybook-viewport-id="mobile2"] #storybook-root {
  padding: 16px;
}

Currently only the height and width of the viewport can be adjusted, but with this change if you want to add other attributes you can define them in CSS without changing the API.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

  1. Run yarn storybook:ui in code\
  2. Open Storybook in your browser
  3. Select a component to preview (such as "Example\Button")
  4. Open the Developer Console run the command: ```javascript
    document.querySelector('iframe[data-is-storybook="true"]').contentDocument.querySelector('body').dataset.storybookViewportId
5. This should print the selected viewport's id (If no viewport is selected it should print the word 'reset'.)
6. Change the viewport and rerun the JS command to see the change.

### Documentation

<!-- Please check (put an "x" inside the "[ ]") the applicable items below to indicate which documentation has been updated. -->

- [X] Add or update documentation reflecting your changes
- [ ] If you are deprecating/removing a feature, make sure to update
      [MIGRATION.MD](https://github.com/storybookjs/storybook/blob/next/MIGRATION.md)

## Checklist for Maintainers

- [ ] When this PR is ready for testing, make sure to add `ci:normal`, `ci:merged` or `ci:daily` GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in `code/lib/cli/src/sandbox-templates.ts`
- [ ] Make sure this PR contains **one** of the labels below:
   <details>
     <summary>Available labels</summary>

     - `bug`: Internal changes that fixes incorrect behavior.
     - `maintenance`: User-facing maintenance tasks.
     - `dependencies`: Upgrading (sometimes downgrading) dependencies.
     - `build`: Internal-facing build tooling & test updates. Will not show up in release changelog.
     - `cleanup`: Minor cleanup style change. Will not show up in release changelog.
     - `documentation`: Documentation **only** changes. Will not show up in release changelog.
     - `feature request`: Introducing a new feature.
     - `BREAKING CHANGE`: Changes that break compatibility in some way with current major version.
     - `other`: Changes that don't fit in the above categories.
   
   </details>

### 🦋 Canary release

<!-- CANARY_RELEASE_SECTION -->

This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the `@storybookjs/core` team here.

_core team members can create a canary release [here](https://github.com/storybookjs/storybook/actions/workflows/canary-release-pr.yml) or locally with `gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>`_

<!-- CANARY_RELEASE_SECTION -->

@yannbf
Copy link
Member

yannbf commented Feb 5, 2024

Hey @jasonsperske thanks for your contribution. This seems like an interesting feature but the way to achieve it might not be the most recommended. Seems like the addon should provide a configuration instead. Else, users might start relying on selectors which are brittle and can break between versions.

Would you be willing to write an RFC regarding the change alongside its use cases? @cdedreuille you might be interested in this as well. Thank you!

@jasonsperske
Copy link
Author

Created an RFC in #25915

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.

3 participants