IW-1022 | global nav #22
Conversation
# Conflicts: # source/components/GlobalNavigation/index.js
# Conflicts: # source/components/GlobalNavigation/components/GlobalNavigationSearch/GlobalNavigationSearchModal.js # source/components/GlobalNavigation/components/GlobalNavigationUser/GlobalNavigationMobileUser.js # source/components/GlobalNavigation/index.js
This reverts commit 156b654.
This reverts commit f7bd2e3.
|
Pinging @JoshuaRogan - CAKE is using their own GN now and it will be NICE to kill that too. |
There was a problem hiding this comment.
Left few comments - I'll add more once I finish detailed CR. Mostly minor stuff.
Few more things:
If you demo GlobalNavigation you need to fix styles of the demo (and probably add a note to README - check FandomContentWell) otherwise this happens:
https://www.dropbox.com/s/x4pbfo3girh6qrg/Screenshot%202019-03-04%2011.58.09.png?dl=0
Also there are errors in the console while running yarn dev:
019-03-04 11:55:41.324|TESTS console.warn node_modules/react-i18next/dist/commonjs/utils.js:17
2019-03-04 11:55:41.324|TESTS react-i18next:: You will need pass in an i18next instance by using i18nextReactModule
And Avatar/AvatarStack/BannerNotification (and maybe others) are missing from the styleguide now Fixed
I'll look at this PR/issues later today.
| @@ -51,6 +51,7 @@ | |||
| "Fieldset", | |||
| "FloatingButton", | |||
| "FloatingButtonGroup", | |||
| "GlobalNavigation", | |||
There was a problem hiding this comment.
Please move GlobalNavigation to systems/ instead` - It's way more complex than just a component.
There was a problem hiding this comment.
I'd argue.
Aside of its size GlobalNav is still a single component with no additional requirements which accepts a bunch of props and does not care about the rest of app, while BannerNotification comes with store injection, actions and a dedicated model.
There was a problem hiding this comment.
I think that is a fair point but GlobalNav is more of an organism whereas components should be molecules/atoms. http://bradfrost.com/blog/post/atomic-web-design/#atoms
There was a problem hiding this comment.
At which moment a 'component' becomes an 'organism'/'system'? It's not clear for me, and could cause some issues with understanding the concept. Eg. if we'd like to move OpenGraph card to common, should it be a 'component' or an 'organism'?
My understanding of the file structure was described above - if you need to export something more than a component itself, it should become a system. But in other case, from dev/react perspective, a component (no matter how big) is still a encapsulated entity that you can use and pass props to.
Maybe to avoid such issues we should rename 'systems' to eg. 'redux' and keep there only related stuff?
|
Still waiting on @JoshuaRogan review (as it's probably going to be used to replace custom GlobalNav in FandomCreator) |
# Conflicts: # docs/build/1.f2aff740.js # docs/index.html # package.json
| @@ -76,6 +77,9 @@ | |||
| "jest/valid-expect": "error", | |||
| "react/button-has-type": 0, | |||
| "jsx-a11y/label-has-for": 0, | |||
| "jsx-a11y/click-events-have-key-events": 0, | |||
There was a problem hiding this comment.
I'm not convinced we should disable all of these. We should try to be accessible as possible at a min in this library. We already do a poor job in most of our apps so it would be good to set the precedent in react-common.
There was a problem hiding this comment.
In general i agree, but this is a company-wide plan that should be mostly driven by UX and product folks, because it requires some specific keyboard behaviours to be introduced. I'd be glad if such thing would become real, but afaik we had some approaches for it and we failed in execution.
Having that rule for the sake of adding empty onKeyPress listeners to satisfy linter did not make sense, so I've decided to disable it after 5th or 6th eslint disable next line comment.
# Conflicts: # docs/build/1.fe780730.js # docs/build/bundle.6126b43e.js # docs/index.html # package.json # source/components/AvatarStack/README.md # source/components/AvatarStack/index.js # source/components/BannerNotification/index.js
This is the main PR for all the work related to implementing our current Ember-based global navigation in React.
Including: