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 placement of notificationistList on docs page #7290

Merged
merged 2 commits into from
Jul 11, 2019

Conversation

lonyele
Copy link
Member

@lonyele lonyele commented Jul 4, 2019

Issue: #7289

What I did

I pass placement object to NotificationList so that it renders inside of Component wrapper.

Now it looks like this. I left last one placement as it is because I thought that is an intended result though I feel like things can be improved
Screenshot_2019-07-04 Storybook(1)

@vercel
Copy link

vercel bot commented Jul 4, 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-notifications-docs-page.storybook.now.sh

@lonyele
Copy link
Member Author

lonyele commented Jul 9, 2019

@shilman @ndelangen (sorry)I didn't' mean to pester you guys, would you review my PRs? If something looks wrong or implementation is bad please let me know. If it is something I can do it, I definitely can apply feedbacks. Because I have 4 PRs right now I'll just write here once for a simple ping.

#7241 I checked that there are not many places using setState, but for this case it is needed?
#7327 There is one chromatic result that looks bad, but on my machine and now deployment looks ok. Is there a possibility that chromatic result can be wrong?(maybe a bug?)
#7200 Everything looks fine except the now deployment. What should I do to fix this one?

@shilman
Copy link
Member

shilman commented Jul 9, 2019

@lonyele Thanks for the ping and sorry for the delay on this. I'll take a look at them tonight.

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

@lonyele
Copy link
Member Author

lonyele commented Jul 9, 2019

Thanks! as you already know. I'm a junior level... I try hard not to take others times away while doing some PRs, but many times it is just a hope...

@ndelangen ndelangen merged commit 926b628 into storybookjs:next Jul 11, 2019
@lonyele lonyele deleted the fix/notifications-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