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

Fix content of Panel rendered wrong at Docs-page #7327

Merged
merged 3 commits into from
Jul 13, 2019

Conversation

lonyele
Copy link
Member

@lonyele lonyele commented Jul 6, 2019

Issue:

What I did

Change the absolute value passed to <Tabs /> to false

More Info

On Canvas viewMode, there is no problem, but only on the docs-page.
I think It can also be found this bug on the deployed one
Screenshot_2019-07-07 Storybook(3)

It used to be the first screenshot below
Screenshot_2019-07-07 Storybook(2)

Now it looks like the second screenshot below
Screenshot_2019-07-07 Storybook(1)

I wasn't sure if it is only not working for me or my implementaion on fixing this problem is breaking other parts of component usage, i made this PR so that I can take a look at how chromatic sees the changes(That's why I put WIP on the title)

First attempt makes the Clear button to the top when there is not content. So big no
Second attempt is to pass absolute to the <Tabs /> component so that in general it is same as before, but its value is false only on the docs-page

I feel this can be a little bit monkey patching, but I couldn't change the logic itself because I wasn't sure why this part was implemented [in this way] (https://github.com/storybookjs/storybook/blob/next/lib/components/src/tabs/tabs.tsx#L65-L87)

btw on <Tabs /> story. Docs-page also looks good, but here <Tabs /> is wrapped by <TabWrapper> so it fixes the position problem?

If this one looks ok, I'll make an issue and clean up little bit for the history's sake

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

@vercel
Copy link

vercel bot commented Jul 6, 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://monorepo-git-fork-lonyele-fix-panel-docs-page.storybook.now.sh

@shilman
Copy link
Member

shilman commented Jul 6, 2019

@lonyele what does this fix? can you attach a screenshot?

@lonyele
Copy link
Member Author

lonyele commented Jul 7, 2019

@shilman Please take a look, I've edited some content for more info

@shilman
Copy link
Member

shilman commented Jul 7, 2019

Thanks @lonyele! Are you able to access the chromatic build? It looks like one of the CSS changes might have broken the keyboard shortcuts screen?

@lonyele
Copy link
Member Author

lonyele commented Jul 7, 2019

@shilman hm... I'm not sure. On my local build, it looks fine on Windows and Ubuntu. Also on now deployment. I checked the React components that are used for the shortcuts page such as this, but it doesn't look like a problem. I think my code change from this PR shouldn't affect other things except the <Panel /> on docs-page. In conclusion.... I don't know why... help is needed here(Is there a possibility that chromatic is wrong?)

@shilman
Copy link
Member

shilman commented Jul 9, 2019

@ndelangen can you take a look?

@lonyele lonyele changed the title [WIP] Fix content of Panel rendered wrong at Docs-page Fix content of Panel rendered wrong at Docs-page Jul 10, 2019
@ndelangen
Copy link
Member

I'm fine with the code change, I think it's low impact.

I did warn inline-rendering of components would have a cascade effects of tweaks to internal components. You know I'm not a fan of this, but I have no reason to deny this change.

I kicked off another CI run, to verify the chromatic diff wasn't a fluke, i suspect it was, since when I look at the page on now, i see the correct layout, not what's on chromatic.

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.

LGTM!

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.

LGTM!

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.

LGTM!

@shilman shilman merged commit 48e5505 into storybookjs:next Jul 13, 2019
@lonyele
Copy link
Member Author

lonyele commented Jul 15, 2019

Please feel free to say anything to me if you guys don't satisfy with the implementation. I have a thirst for feedbacks... since I'm a self-taught programmer(I'll try to meet your satisfaction if I can!)

@lonyele lonyele deleted the fix/panel-docs-page branch July 19, 2019 05:40
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