web: better unify all the trigger buttons #5261
Conversation
|
This pull request has been linked to Shortcut Story #13060: auto_init=False does not show trigger button. |
|
there's still a bunch of copy-and-pasted code between SidebarTriggerButton and OverviewTriggerButton, but i tried to make it so at least the code is the SAME copy-and-pasted code 😬 |
There was a problem hiding this comment.
happy to see some code cleanup with a bug fix! 🎉
is there any reason in the future to keep these as separate components? the styling is slightly different between the two views, but at first glance, i don't see major functionality differences. (asking for my curiosity, and not because i think this pr should do more.)
i may create a backlog ticket for myself to clean up the hover/focus states because i don't think the colors are accessible, and i could tackle other cleanup.
| shouldBeClicked = true | ||
| } else if (!props.hasBuilt && isManualTriggerIncludingInitial) { | ||
| shouldBeClicked = true | ||
| let isBold = false |
There was a problem hiding this comment.
(nit) i found isBold as a var name a little confusing without being too familiar with all the trigger button states. wdyt about isEmphasized instead? (i think it's fine that the css class is isBold.)
| if (props.hasPendingChanges && isManual) { | ||
| isBold = true | ||
| } else if (!props.hasBuilt && !isAutoInit) { | ||
| isBold = true |
There was a problem hiding this comment.
fyi, there is a slight discrepancy between table view and detail view when a manual resource has been triggered once, even though it looks like this logic is the same! i did check on the main branch and it seems like this is happening there too, so it's not really anything to fix here.
detail view fills the icon with blue:
table view does not (but it is blue when the resource is manual init and has not been built):
There was a problem hiding this comment.
haha -- yahhhhh, there are a bunch of discrepancies here. honestly i meant this to be a 2-line PR but it was hard to resist not fixing some of them (tho there are still some left)
|
👍 |
|
speaking to: "is there any reason in the future to keep these as separate components?" i tend to lean on the side of https://sandimetz.com/blog/2016/1/20/the-wrong-abstraction - i.e., to bias towards code duplication if you're not sure how to come up with the right abstraction for an attribute. tho reasonable people differ on this. i wouldn't be opposed to trying to unify them, but also wouldn't take for granted that it would be an improvement. |
Hello @lizzthabet,
Please review the following commits I made in branch nicks/ch13060:
e85f34f (2021-12-08 09:24:09 -0500)
web: better unify all the trigger buttons
fixes #5252
Code review reminders, by giving a LGTM you attest that: