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

web: add custom navbar button support #4723

Merged
merged 4 commits into from Jul 2, 2021
Merged

Conversation

milas
Copy link
Member

@milas milas commented Jul 1, 2021

Render Global:nav buttons in the global nav. User-defined buttons
show up first, then the built-ins (Tilt version, snapshot, help,
account).

2021-07-01 16 04 18

The backend enforces that Global:nav buttons have an icon defined,
though there is no practical way on either backend or frontend to
determine that the icon name is valid; if it's not a real material
icon, you'll get a blank space, so it should be fairly straightforward
to debug regardless.

Log output (via backend) ends up in the (global) span viewable by
"All Resources" in the web interface.

@milas milas requested review from hyu, nicks and lizzthabet July 1, 2021 20:11
@milas milas self-assigned this Jul 1, 2021
@milas
Copy link
Member Author

milas commented Jul 1, 2021

I'm going to be honest, I am not a wizard with styled-components. I tried to do what I thought was logical here in terms of making the ApiButton component re-usable since it has the logic for analytics + update API with last click (to actually trigger the command).

I basically had to convert a couple places to mix-ins so that I could re-use them across styled components. I also made the ApiButton by default render $icon $text but if children are passed in via props, that takes precedence for content, so that the nav version can send its custom MenuButtonLabel for the hover text.

If this is all totally wrong/insane and there's a better way to do things, let me know and I can happily re-work this PR!

<InstrumentedButton
analyticsName={"ui.web.uibutton"}
onClick={onClick}
disabled={loading}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wonder if it makes sense to always disable the button if pathBuilder.isSnapshot(), so the button doesn't do anything in snapshots

Copy link
Member

@nicks nicks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i am...slightly worried about the multi-dimensional venn diagram of mixins. I'm not sure I have a better suggestion right now.

My instinct is that React component composition is great for expressing UI hierarchy (i.e., box A visually contains box B).

We probably shouldn't be using it for multiple-inheritance of functionality and data-flow (in particular, ApiButton and InstrumentedButton are examples of this pattern).

but that pattern wasn't introduced in this PR, so i think it's fine to punt on this for now and Han + Lizz + I can talk more about it next week

@milas
Copy link
Member Author

milas commented Jul 2, 2021

multi-dimensional venn diagram

Yeah, that was precisely my concern - in no world is this sane, but I wasn't sure of a good alternative that wouldn't spiral scope. My only other thought was not share components at all but have some sort of CreateAPIButtonClickHandler function they could both use since it's really the onClick logic we need to share, but that also felt very un-React.

@milas milas force-pushed the milas/uibutton-navbar-types branch from 5e1fe28 to fd6903a Compare July 2, 2021 15:13
* Optional `icon` field - will be primary for navbar buttons with
  `text` as hover; for resource icons, will be alongside
* New constant for component type: `Global` (expected component ID
  for navbar will be `nav`)
@milas milas force-pushed the milas/uibutton-navbar-types branch from fd6903a to 6bd0a32 Compare July 2, 2021 15:27
@milas milas force-pushed the milas/web-uibutton-navbar branch from b69461a to 03e3ce7 Compare July 2, 2021 19:11
@milas
Copy link
Member Author

milas commented Jul 2, 2021

Had to rebase for API changes, but committed SVG changes independently as 03e3ce7. I did have to pull in a new small package to handle safely creating <svg> DOM elements dynamically. (Can't just <img src="data:..." /> because then you can't style with CSS, so plain path SVGs won't get automatically colored for the UI theme.)

@milas
Copy link
Member Author

milas commented Jul 2, 2021

Actually, hold on - I'll back out those changes from this PR and open a separate PR with just SVG support 😬

Render `Global:nav` buttons in the global nav. User-defined buttons
show up first, then the built-ins (Tilt version, snapshot, help,
account).

The backend enforces that `Global:nav` buttons have an icon defined,
though there is no practical way on either backend or frontend to
determine that the icon name is valid; if it's not a real material
icon, you'll get a blank space, so it should be fairly straightforward
to debug regardless.

Log output (via backend) ends up in the `(global)` span viewable by
"All Resources" in the web interface.
@milas milas force-pushed the milas/web-uibutton-navbar branch from 03e3ce7 to 5bc1ea9 Compare July 2, 2021 19:16
Base automatically changed from milas/uibutton-navbar-types to master July 2, 2021 19:21
@milas milas added the ui label Jul 2, 2021
@milas
Copy link
Member Author

milas commented Jul 2, 2021

Sorry for Git chaos here, just wanted to unbundle the changes - this is now the same as when originally approved minus dealing with codegen merge conflicts. See #4725 for SVG support.

@milas milas merged commit cd45491 into master Jul 2, 2021
@milas milas deleted the milas/web-uibutton-navbar branch July 2, 2021 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants