feat: new flag info box #9308
feat: new flag info box #9308
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files |
There was a problem hiding this comment.
Nice one! Got a couple questions and comments throughout but nothing blocking, so I'm gonna give this a ✅ and you can address them either now or later (or never if you disagree).
Regarding the screenies in the PR description: are the tags supposed to wrap like they do in the final, light screeshot? It looks a little weird to me that they're suddenly left-aligned. I don't expect we have sketches for that, so we should probably check in with UX.
.../src/component/feature/FeatureView/FeatureOverview/FeatureOverviewMetaData/DependencyRow.tsx
Outdated
Show resolved
Hide resolved
| ) : null} | ||
| {hasParentDependency && !feature.dependencies[0]?.enabled ? ( | ||
| <StyledMetaDataItem> | ||
| <StyledMetaDataItemLabel> | ||
| Dependency value: | ||
| </StyledMetaDataItemLabel> | ||
| <span>disabled</span> | ||
| </StyledMetaDataItem> | ||
| ) : null} |
There was a problem hiding this comment.
Not very important, but can we use && instead of ternaries in this file to get rid of all the : null? It's kinda distracting when there's this many of them, I think.
There was a problem hiding this comment.
We don't have reliable way of catching issues related to && operator. https://docs.getunleash.io/contributing/ADRs/front-end/jsx-conditionals#pitfalls-of--operator
| </StyledMetaDataItemLabel> | ||
| <StyledMetaDataItemText data-loading> | ||
| {project} | ||
| {capitalizeFirst(type || '')} flag |
There was a problem hiding this comment.
This can also be done by CSS ::first-letter: { text-transform: uppercase } which will also capitalize "flag" if there is no type 💁🏼 Not important, but this feels more like a CSS thing to me 🤷🏼
There was a problem hiding this comment.
I'm using what we already have, plus I did not have good time debugging and finding what's causing changes when using :: pseudo-classes in the past.
| <StyledMetaDataItem data-loading> | ||
| <StyledMetaDataItemText> | ||
| {description} | ||
| </StyledMetaDataItemText> | ||
| </StyledMetaDataItem> | ||
| } | ||
| /> | ||
| ) : null} |
There was a problem hiding this comment.
Same question regarding ternaries vs double ampersands. Wouldn't && be easier? But did we say not to use double ampersands?
frontend/src/component/feature/FeatureView/FeatureOverview/FeatureOverviewMetaData/TagRow.tsx
Outdated
Show resolved
Hide resolved
| @@ -99,80 +96,77 @@ export const TagRow = ({ feature }: IFeatureOverviewSidePanelTagsProps) => { | |||
| } | |||
| }; | |||
|
|
|||
| const addTagButton = ( | |||
There was a problem hiding this comment.
Should this be a function component and rendered as <AddTagButton/> instead?
I thought of the "Never call function component functions directly" react docs, but this is a variable, so I'm not sure whether it applies here or not 🤔
There was a problem hiding this comment.
This doesn't apply for things rendered into a variable. But thanks for pointing it out. I refactored it to a separate component
| <StyledMetaDataItem> | ||
| <StyledLabel>Tags:</StyledLabel> | ||
| <StyledTagContainer>{addTagButton}</StyledTagContainer> | ||
| </StyledMetaDataItem> | ||
| ) : ( | ||
| <StyledTagRow> | ||
| <StyledLabel>Tags:</StyledLabel> |
There was a problem hiding this comment.
Small point, but is there a reason why you swap out the metadata item container here?
In fact, I'd expect these two branches to be exactly the same: Container -> Label + Tag Container -> Tags + Button. So if the list of tags is empty, then tags.map gives you no components back and it'll still work.
Does that check out?
There was a problem hiding this comment.
It didn't align tags properly, because we have 2 cases here: if tags fit in one line, keep it this way (aligned to the right), but if it doesn't fit - align to the left in starting from next line.
I'll spend 5min checking if I can make it work.
…tureOverviewMetaData/TagRow.tsx Co-authored-by: Thomas Heartman <[email protected]>
New design:
What's not done yet: