feat: add resource-action pills to custom roles table #14354
Conversation
|
🤖 Meticulous spotted visual differences in 91 of 1477 screens tested: view and approve differences detected. Meticulous tested 111/112 of the executable lines edited in this PR1. 1. These lines will likely automatically gain test coverage over the coming days, however if you wish to increase coverage immediately you can do so by interacting with your feature on localhost. Last updated for commit d21771f. This comment will update as new commits are pushed. |
site/src/theme/light/permission.ts
Outdated
| background: colors.zinc[200], | ||
| outline: colors.zinc[300], | ||
| text: colors.zinc[700], | ||
| } satisfies Permission; |
There was a problem hiding this comment.
I'm wondering if permission should be treated as a theme or not as a design token in this case, but I'm okay with it for now. Perhaps having a 'muted' pill variant could be better. 🤔 Wdyt @aslilac ?
|
I have reviewed this pull request, and it is functioning as expected. It looks great! Good work. However, it still requires tests and another review for usage permissions as a design token. |
site/src/theme/permission.ts
Outdated
| export interface Permission { | ||
| background: string; | ||
| outline: string; | ||
| text: string; | ||
| } |
There was a problem hiding this comment.
Imho, this is way too specific of a thing to add to the theme. We've been missing a clear "neutral" role choice for a while, and I've been meaning to resolve that for a while but it just hasn't come up recently. That being said, you got me thinking about it again today and I have proposed a fix in #14356. If that PR gets merged, I'd much rather use theme.roles.info colors for these than complicate the global Theme for this one specific use case.
There was a problem hiding this comment.
I really just added the Permission theme to have a short discussion about it. I was originally going to use or modify role but semantically permissions are not roles (roles are assigned permissions) and if we are using a feature concept such as roles in the theme it would make sense to add permissions as well. And this will also make it more likely for others in the future to have the idea to add theming for features (like I did) then adding theming for UI concepts. For example, this could happen with other elements that have a lot of variations such as avatars, buttons, cards, status indicators, etc.
The other way to do it is more like in mui.ts where its about UI elements and tags. If we following that line of thinking then we could use the name pills in the theme instead of roles. In this case we are thinking more about the UI component itself than the feature it is used for.
There was a problem hiding this comment.
Sorry about the confusion. The theme.roles property is entirely unrelated to the "Roles" feature in the product. 😄 #14356 has been merged now though, so theme.roles.info is there to use as a neutral color set.
There was a problem hiding this comment.
theme.roles.info.background and theme.roles.info.outline are different than the colors I want to use. Where would you suggest placing the colors I wanted to use from theme.roles.default?
b3c3361 to
9879c09
Compare
site/src/theme/colorRoles.ts
Outdated
| export interface Roles { | ||
| export interface ColorRoles { | ||
| /** The default color role for general use */ | ||
| default: ColorRole; |
There was a problem hiding this comment.
I'm gonna be defensive about this because I put a lot of thought into the design of this bit of our theme: default, the way that you are using it, is not a UI role. As a user, when the app uses the error colors I can say "The UI is showing me something is wrong", but I can't say "The UI is showing me a default color role for general use". I think the intent of these roles is really important for consistency throughout the product.
I'm also not convinced that ColorRole is a better name. The colors they represent are a consequence, not the primary motivation. Otherwise they'd just be named red, orange, etc. You're supposed to think about what you're trying to communicate with the color first, and then the colors come from that naturally. Maybe UiRole or something if you really think it needs to be changed.
default could potentially be an appropriate role name if it was identifying things in the UI as a default target. The default org, the default template version, so on. But there isn't anything about the permissions given to an org role that makes them "default" of that role, especially not when used to highlight all of them.
There was a problem hiding this comment.
What I want is a way to define styles for a default pill, and although info can work, the name feels like its a special case and not the standard/default color. I would ideally like to keep defaults in palette or someplace outside of roles in case it doesn't fit into background, outline, text, or the fill colors.
|
@aslilac If I add a color to experimental, do I just give it a descriptive name next to l1 and l2? or should it go inside l1 or l2? If l1 means primary and l2 means secondary, how do those colors get translated elsewhere once they leave experimental? |
|
as long as it's inside |
e0542c9 to
de5e171
Compare
| export const PermissionPillsList: FC<PermissionPillsListProps> = ({ | ||
| permissions, | ||
| }) => { | ||
| const resourceTypes: string[] = getUniqueResourceTypes(permissions); |
There was a problem hiding this comment.
is this type annotation here still necessary? maybe if TypeScript is having trouble inferring the type here you could add : string[] to the function declaration
There was a problem hiding this comment.
not necessary, just leftover from implementation work.
resolves #14247