AppBar Component #378
AppBar Component #378
Conversation
|
Thanks John - looking forwards to checking this out shortly. I was thinking I would take a run through the current hoist-react apps and migrate them to a remote switcher, but I think it makes more sense to see if we can take an even deeper standardization / cleanup with this component. Will post any comments or questions shortly. |
There was a problem hiding this comment.
Looks great. Let me know what you think of the small comments I've added.
Re. leaving off refresh button - I agree that sounds right for now. Having a standard app-wide refresh affordance has been helpful in the past, and we might wish to continue developing that idea - but it's admittedly also a bit weird.
If we did want to standardize, I think we would do so via HoistAppModel exposing a method like globalRefresh that fires an event.
Edit - although for apps that do have such a button now, the current code would force it to move to the left of the admin/theme button... Not clearly bad, but a change and I do think I prefer it where it is now on the far right (but I also seem to prefer other left-side controls to be to the left of the other buttons - just to make things difficult 👿 )
| }); | ||
| } | ||
|
|
||
| onContactClick = () => { | ||
| window.open('https://xh.io/contact'); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Have wanted to ask what we wanted as our standard here for semicolons after class properties. I think adding them is most consistent with our general standards.
As per tc39/proposal-class-public-fields#25 it seems like it's generally more correct and avoids relying on ASI.
I took a quick look at our eslint rules and not entirely sure why it's not flagging them - https://github.com/exhi/hoist-dev-utils/blob/82d45641a1f9016c720e5e3a73a19322b4520e3f/packages/eslint-config/index.js#L83
Anyway - we should file a separate ticket to bomb the code and standardize if we go this way, and ideally get eslint to flag.
There was a problem hiding this comment.
I have been on the fence about this. It's inconsistent but would be happy to skip the semis on the fat arrows that are acting as bound methods
cmp/tab/TabSwitcher.js
Outdated
| @@ -39,7 +39,8 @@ export class TabSwitcher extends Component { | |||
| onChange: this.onTabChange, | |||
| large: !vertical, | |||
| selectedTabId: selectedId, | |||
| items: children.map(({id, name}) => tab({id, title: name})) | |||
| items: children.map(({id, name}) => tab({id, title: name})), | |||
| ...this.props | |||
There was a problem hiding this comment.
This we should pull the model off of the props to avoid passing that through to the underlying Blueprint tabs component (in case it decided to complain about an unknown prop or, worse, declare its own model prop with a different meaning).
cmp/appbar/AppBar.js
Outdated
| /** Set to true to hide the Theme Toggle button. */ | ||
| hideThemeButton: PT.bool, | ||
| /** Set to true to hide the Logout button. */ | ||
| hideLogoutButton: PT.bool |
There was a problem hiding this comment.
We could call out in this comment that the button won't render for apps that don't support logout (as specific in their AppModel), to avoid confusion for these SSO-enabled apps.
admin/App.js
Outdated
| tabSwitcher({ | ||
| model: this.model.tabs, | ||
| large: false | ||
| }) |
There was a problem hiding this comment.
The one feature question that's been in my mind re. this comp is if it should also bundle up the creation of a tabSwitcher - could support a tabContainerModel prop that, if given, creates this automatically. That might be a useful convenience and help to further standardize. (Also looking at the need for large: false here which is a bit unfortunate if we do want to use this pattern frequently.)
There was a problem hiding this comment.
Yeah was thinking the same, makes sense to standardize on the main app nav switcher I think.
Re: large: false on the tabSwitcher, I'd prefer that we not default to large tabs in tabSwitcher. I kept it in there when I did the TabContainer re-work. Seems like it was mostly there because the top-level tabs were typically used for the app nav, with potentially 1 level of nesting using vertical tabs, so the larger horizontal tabs made sense. Any objection to just taking large tabs for horizontal out of tabSwitcher?
There was a problem hiding this comment.
I am onboard with the default being "normal" sized tabs and you need to ask for large tabs.
With our current plan of migrating existing projects to use a tabSwitcher in the top app bar for overall navigation, any other tabsets in the app are more likely to be secondary things that want a normal sized switcher anyway.
|
Merging this now to include it in 7.0 release this evening or tomorrow. Lee I am sorry if this rushes a second review - of course we can loop back but I think the updates John made are ones we all generally agreed upon. |
#307
I left the built-in refresh button out since there isn't yet a consistent way to refresh an app, seems like that is a different story and the standard button can be added to AppBar once that mechanism is decided on.
I also updated the Admin app to put the TabSwitcher in the AppBar.
See xh/toolbox#18 for toolbox usage of the new component