Add initial site layout component #15
Conversation
jamaljsr
requested review from
a team and
guggero
and removed request for
a team
jamaljsr
force-pushed
the
feat/storybook
branch
from
bae685f to
b0728c6
Compare
jamaljsr
force-pushed
the
feat/storybook
branch
from
b0728c6 to
3e47922
Compare
|
I added a couple of commits just to improve and refactor the styles |
|
I refactored the code a bit more to clean it up a bit for mobx, storyboard and unit tests. |
jamaljsr
force-pushed
the
feat/storybook
branch
from
2cf6319 to
07dbdab
Compare
| @@ -0,0 +1,20 @@ | |||
| import React from 'react'; | |||
There was a problem hiding this comment.
These example stories are deleted in the next commit. Can we just not add them at all to reduce the diff size?
There was a problem hiding this comment.
Sorry about that. I will be mindful of this in the future. Thanks for the feedback.
There was a problem hiding this comment.
No problem! It's always a question of how much time is worth putting into editing commits. On a new and not-yet-published project we probably don't have to be as strict in reviewing as in lnd for example and it's more important to get things moving.
| import Sidebar from './Sidebar'; | ||
|
|
||
| const Styled = { | ||
| Background: styled.div` |
There was a problem hiding this comment.
This is pretty cool! Just out of curiosity: I noticed that this creates random class names in the rendered code, which makes it pretty hard to debug/inspect. Is there an easy debug mode that can be enabled that creates more verbose class names?
There was a problem hiding this comment.
Yes, it is possible to add more descriptive labels in the class names generated by emotionjs. It will require adding a babel plugin which is only possible by also adding the customize-cra package, since create-react-app prevents babel configuration changes by default. I will make this change today.
There was a problem hiding this comment.
After looking into this a bit more, I realize that it is does add the labels into the css classes without needing any additional third-party dependencies. Unfortunately, there is currently a bug when using emotionjs with a typed theme prop. This is why the labels aren't showing up in our app. The bug fix will be released in the next version, so I think it would be better to wait for their next release instead of adding more deps.
There was a problem hiding this comment.
Sure, this isn't a blocker at all. I think from context it should still be possible to debug CSS issues, it might just be a bit more tricky. But since the app isn't too complicated yet it shouldn't be an issue.
Closes #4 & #7
This PR adds the initial shared layout component which contains the navigation bar and will wrap all the different screens on the site. It will continue to evolve as more components are created, but this should be enough get started.
I also installed and configured the Storybook dev tool which will make building and testing UI components much easier. Just run
yarn storybookin theapp/folder to view the stories in a browser.