fix: resource card editable name pencil css fix #715
Conversation
ChitlangeSahas
requested review from
juliajames12,
mavarius and
a team
as code owners
| @@ -158,7 +158,7 @@ export const ResourceCardEditableName = forwardRef< | |||
| onClick={handleClick} | |||
| data-testid={testID} | |||
| > | |||
| <span>{name || noNameString}</span> | |||
There was a problem hiding this comment.
this span was part of the issue.
| @@ -19,8 +19,8 @@ | |||
| position: relative; | |||
| display: flex; | |||
| flex-wrap: nowrap; | |||
| align-items: center; | |||
There was a problem hiding this comment.
Dont need it in center, rather, at the start of the flex.
There was a problem hiding this comment.
one thing to be aware of when modifying CSS is where this class is used. I did a quick poke around the UI (in the chrome console, using $('.cf-resource-editable-name) will show you if there are any elements that match that class selector) and found a few places like in paginated tasks and paginated tokens where this class is used. Will justifying the content to flex-start mess things up there?
I did notice that changing align-items: center changed the vertical placement of text. Do you know what happens when you keep align-items: center and add justify-content: flex-start?
There was a problem hiding this comment.
Changing a CSS selector is one of the more difficult parts of maintaining CSS because it can sometimes be hard to understand how often it's used. That difficulty is compounded when it's used in a component library like Clockface. Depending on how thoroughly you tested, we may want to tweak this or test more parts of the app.
| @@ -19,8 +19,8 @@ | |||
| position: relative; | |||
| display: flex; | |||
| flex-wrap: nowrap; | |||
| align-items: center; | |||
There was a problem hiding this comment.
one thing to be aware of when modifying CSS is where this class is used. I did a quick poke around the UI (in the chrome console, using $('.cf-resource-editable-name) will show you if there are any elements that match that class selector) and found a few places like in paginated tasks and paginated tokens where this class is used. Will justifying the content to flex-start mess things up there?
I did notice that changing align-items: center changed the vertical placement of text. Do you know what happens when you keep align-items: center and add justify-content: flex-start?
| font-size: $cf-card--name-font-size; | ||
| font-weight: $cf-card--name-font-weight; | ||
| font-family: $cf-text-font; | ||
| display: inline-block; |
There was a problem hiding this comment.
this selector is also used in more places than dashboards and is something you'll want to take great care changing. Changing from inline-block to flex might have very odd changes in layout. It might be better to try inline-flex, but even then, I'd be very careful changing that, as it might have some strange, unintended consequences beyond just the cards you tested them in.
|
I understand the concerns above, to best navigate the issue of making sure that this doesn't break anything I went through and tested all the pages that have the resource card, I checked:
Rest of the app has a different flow of renaming, (no pencil, but through the cog icon there is the rename option). There seem to be no issues. cc: @hoorayimhelping |
Part of #702
Changes
Changes the CSS so that the icon is always aligned with the first line of the name.
Screenshots
Checklist
Check all that apply