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

[Bug]: Docs story wrapping issue with fixed positioned elements #23586

Open
konsalex opened this issue Jul 25, 2023 · 16 comments
Open

[Bug]: Docs story wrapping issue with fixed positioned elements #23586

konsalex opened this issue Jul 25, 2023 · 16 comments

Comments

@konsalex
Copy link
Contributor

konsalex commented Jul 25, 2023

Describe the bug

Positioning in Docs page is broken for elements with fixed positioning.

Examples:
Working deployment (7.0.6): https://storybook-7-0-6-reproduction.surge.sh/?path=/docs/uicomponents-date-picker-date-range--docs

Issue (7.1.0): https://storybook-7-1-0-reproduction.surge.sh/?path=/docs/uicomponents-date-picker-date-range--docs

image

Also found another comment here describing the same issue:
#16774 (comment)

To Reproduce

Follow this reproduction link:
https://codesandbox.io/p/sandbox/storybook-7-0-6-7-1-0-docs-issue-vkvwwy?file=%2Fpackage.json%3A21%2C27

You can switch the version 7.1.0 to 7.0.6 to reproduce this issue.

System

System:
    OS: Linux 5.15 Debian GNU/Linux 11 (bullseye) 11 (bullseye)
    CPU: (2) x64 AMD EPYC
  Binaries:
    Node: 16.17.0 - /usr/local/bin/node
    Yarn: 3.6.1 - /usr/local/bin/yarn
    npm: 8.15.0 - /usr/local/bin/npm
  npmPackages:
    @storybook/addon-essentials: 7.1.6 => 7.0.6 
    @storybook/addon-interactions: 7.1.6 => 7.0.6 
    @storybook/addon-links: 7.1.6 => 7.0.6 
    @storybook/addons: 7.1.6 => 7.0.6 
    @storybook/react: 7.1.6 => 7.0.6 
    @storybook/react-vite: 7.1.6 => 7.0.6

Additional context

Seems in <7.1.0 zoom was used, which was changed to transform and scale making this break 🤔

@valentinpalkovic
Copy link
Contributor

This bugfix has introduced the current behavior: #21303

I'm honestly a little conflicted. On the one hand, I understand your problem, but on the other hand, I believe the container inside the docs page should be decapsulated from the environment, and elements should not break out of it. Now, you could adjust the height of the container on the Docs page so that modals find a place there as soon as they are triggered. What do you think? Do I overlook something?

cc @vanessayuenn, @shilman WDYT?

@konsalex
Copy link
Contributor Author

I am conflicted myself @valentinpalkovic . To be honest I think this is the right way to go, as zoom's behavrior is inconsistent between browsers, and we have issues with Chrome vs Firefox vs Safari.

At least if we know that transform will be used everywhere we could act as you suggested, and make the container bigger, or portal the elements to the document.body as work arounds.

@vanessayuenn
Copy link
Contributor

Thanks for flagging this. I'm inclined to agree that this is the "more correct" behaviour and the workaround as @valentinpalkovic suggests is adequate. I'm going to close this for now, but feel free to reopen if you think there's something else to be done here!

@vanessayuenn vanessayuenn closed this as not planned Won't fix, can't repro, duplicate, stale Jul 25, 2023
@konsalex
Copy link
Contributor Author

@vanessayuenn would you consider having a more fine-grained control of zooming feature for docs, so people could disable it in elements like popovers and context menus, that fixed positioning is a frequent use case

@stefanpearson
Copy link

Thanks for flagging this. I'm inclined to agree that this is the "more correct" behaviour and the workaround as @valentinpalkovic suggests is adequate. I'm going to close this for now, but feel free to reopen if you think there's something else to be done here!

I don't think this is the actual issue here, and is not to do with an element fitting in to the story box, but instead, any component utilising fixed positioning loses its position (i.e. a popover). This is because the coordinates are no longer relative to the viewport, and break when a transform is applied to the container.

@Luk-z
Copy link
Contributor

Luk-z commented Jul 29, 2023

@vanessayuenn would you consider having a more fine-grained control of zooming feature for docs, so people could disable it in elements like popovers and context menus, that fixed positioning is a frequent use case

@konsalex can you please write an example about how you expect to use the "fine-grained control of zooming feature for docs".

I'm totally agree with @valentinpalkovic, the correct behavior of the docs container is to simulate a separate viewport.
Consider this scenario: a Modal stories with a lot of fixed/absolute modals in the docs, all modals will be rendered on top of each other.

If you have a story with fixed element you have to consider the height and the position of that fixed element to be correct displayed in the docs.

@konsalex
Copy link
Contributor Author

konsalex commented Aug 1, 2023

@Luk-z not sure exactly about the internals and what would be the best option here.

But usually the components that use fixed positioning are very few in a Component Library, so I would expect this setting to be inside a CSF rather than the Preview file.

Would a parameter on the component meta work? I can see also from docs that some addons may rely to parameters.

Example usage I am thinking:

export default {
  title: 'UIComponents/Dropdown',
  component: Dropdown,
  tags: ['docsPage'],
  parameters: {
    disableZoom: true // Zoom is enabled by default  
  },
} as Meta<ComponentProps<typeof Dropdown>>;

Usability wise, the best would be to also include a message on why we disable zoom in specific components, so users will not be confused by having some stories with and some without zoom.

Last, maybe we could use a global parameter in Preview file if we want this feature disabled everywhere in our Storybook.

@Luk-z
Copy link
Contributor

Luk-z commented Aug 1, 2023

Ok, but this definitley not a bug, maybe open a feature request?

@konsalex
Copy link
Contributor Author

konsalex commented Aug 3, 2023

@Luk-z added this also as a feature request: #23699

@vanessayuenn
Copy link
Contributor

Thank you for this discussion. I'll be closing this issue for now and let's keep tracking the feature request in #23699.

@vanessayuenn vanessayuenn closed this as not planned Won't fix, can't repro, duplicate, stale Aug 22, 2023
@etlovett
Copy link
Contributor

etlovett commented Jan 8, 2024

I'm upgrading from v6.5.13 to v7.6.7 for a relatively large design system that uses Storybook to generate a documentation site for internal usage, and I'm finding this issue to be a major headache during this upgrade. Lots of different types of components used to overflow their canvas bounds in our docs: dropdowns, menus, tooltips, modals, takeovers, dialogs, etc. For any component that uses any of these features, we must now specify a hardcoded larger height for the story. These hardcoded values have to then be hand-tuned for nearly every story, to ensure that the story contents fit in the canvas without excessive extra whitespace in our docs. We also can no longer test modals and takeovers full-screen to properly see behavior like the background page scroll becoming disabled.

Please consider reverting this change or adding some way to allow stories to overflow their canvas bounds to allow us to opt out of this hassle.

Thanks for an otherwise-great product!

@valentinpalkovic
Copy link
Contributor

@shilman, @vanessayuenn Should we tackle this topic for 8.0?

@shilman
Copy link
Member

shilman commented Jan 8, 2024

@valentinpalkovic I'd say if it's not a breaking change we should punt to 8.1. We can accept PRs earlier or do it in cooldown if time permits.

@etlovett
Copy link
Contributor

etlovett commented Jan 8, 2024

if it's not a breaking change

You are of course welcome to make whatever decision you want about prioritization, but FWIW I think it would be reasonable to argue that the original change in v7.1.0 was a breaking change. It didn't change any programmatic API contract, but in a way it did change the UI contract.

You might also consider mentioning it in the migration file for anyone who still hasn't migrated across the v7.1.0 boundary.

@Saurabhmagicsw
Copy link

Saurabhmagicsw commented Apr 7, 2024

Hi all and @konsalex , you can use this css in storybook

.docs-story div[scale="1"] {
  transform: none;
} 

working fine for me

larskrj pushed a commit to helsenorge/designsystem that referenced this issue Apr 16, 2024
… vises feil i docs-fanen i storybook

Bugen er kjent for Storybook, men de har ikke implementert noen fiks enda: storybookjs/storybook#23586
Det er flere som har laget workarounds, en er å rendre story som en iframe istedenfor inline ved å sette inline false https://storybook.js.org/docs/api/doc-block-story#inline , som foreslått i denne tråden storybookjs/storybook#8011 .

Det er noen ulemper med dette, lettest å vise i code review

Related work items: #322120
@dev-nicolaos
Copy link

We went with a slightly different selector to avoid impacting anything rendered inside the story.

.docs-story :not(.sb-story *) {
  transform: none;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Empathy Backlog
Status: No status
Development

No branches or pull requests

9 participants