[CP Staging] Fix: align badge in workspace list #40599
Conversation
|
@cubuspl42 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
src/styles/index.ts
Outdated
| @@ -3207,6 +3207,20 @@ const styles = (theme: ThemeColors) => | |||
| marginLeft: 3, | |||
| }, | |||
|
|
|||
| WorkspaceRightColumn: { | |||
| marginLeft: 99, | |||
There was a problem hiding this comment.
Hmm why are we taking this kind of approach with hard-coded values instead of just using flex to fill up space?
There was a problem hiding this comment.
Where did you get 99 from?
There was a problem hiding this comment.
width of the badge
the value was 40 which is the with of the 3 dots menu
There was a problem hiding this comment.
I can use 100 or any higher value
There was a problem hiding this comment.
At the table header, we have a non-visible 4th column, all the 1st 3 columns use flex to stretch on the space equally. and we do hard-coded spacing for the 4th column to make sure it takes the lowest space.
I set the value to 100 to fit the badge in some cases and the 3 dots menu.
There was a problem hiding this comment.
Hmm but you can tell that the columns in the header don't perfectly match up with the content below. Owner looks like it's too far to the left for instance, compared to the content below it.
Why wouldn't that invisible 4th column header just match the width of the 4th column in the table?
There was a problem hiding this comment.
This is a screenshot from stagging
As you can see, under Owner, the content has some margins, that push the content to the right a bit, and this is why you see
the header don't perfectly match up with the content below
There was a problem hiding this comment.
Right, but if there is no badge present, that means we just have a bunch of dead space?
There was a problem hiding this comment.
Yes and No
Yes because technically it's true.
No because the space is already empty:
and if you think we need the space in small screens, the layout change on small screens to
So I think it's not a wasted space.
But if you want, I can create a logic to find out if there well be a badge for Request, then adjust this value 100/40
src/styles/index.ts
Outdated
| }, | ||
|
|
||
| workspaceThreeDotMenu: { | ||
| marginLeft: 59, |
There was a problem hiding this comment.
Where does 59 come from?
There was a problem hiding this comment.
99 (badge width) - 40 (3 dots width)
src/styles/index.ts
Outdated
| @@ -3207,6 +3207,20 @@ const styles = (theme: ThemeColors) => | |||
| marginLeft: 3, | |||
| }, | |||
|
|
|||
| WorkspaceRightColumn: { | |||
| marginLeft: 100, | |||
There was a problem hiding this comment.
Can you show me exactly what this looks like? I don't understand why we are hard coding so much margin. Does that mean we have 100px of space that can only be occupied by margin? How does that work when the viewport is bigger or smaller? It just feels like these hard coded margins are a hack and don't provide good responsiveness.
This comment has been minimized.
This comment has been minimized.
mountiny
changed the title
There was a problem hiding this comment.
I agree with @shawnborton that this logic feels fragile, I can assure you that if someone changes the width of the badge or adds a badge of a different width, they will bring this bug back.
Could you please try to find solution which wont depend on the width of the badge?
src/styles/index.ts
Outdated
| @@ -3207,6 +3207,20 @@ const styles = (theme: ThemeColors) => | |||
| marginLeft: 3, | |||
| }, | |||
|
|
|||
| WorkspaceRightColumn: { | |||
There was a problem hiding this comment.
| WorkspaceRightColumn: { | |
| workspaceRightColumn: { |
|
@mountiny @shawnborton This layout and the way to use content width value was there, I didn't create it. I just changed the value from 40 to 100. 40 was the width of the 3 dots menu. The idea from this aspect, is to use 4 columns keeping the 4th one small as possible. To solve the issue you mentioned, we have 2 main solutions: 1- Use flex for the 4th column as it's been used for the other 3 columnsThis solution creates 4 equal columns, equal in width. The issue here there will be a more empty space between the "Workspace type" and the final 4th column.
2- change the table to only 3 columnswhere the action will be part of the 3rd column but aligned to the right.
What do you think? I like solution 2 |
|
@dragnoir I think the option 2 is quite elegant, how does it scale on other screensizes? |
Same, I will update the layout to keep it as it us now (fixed) |
|
Lets watch out to make sure the style changes will not influence any other components |
|
@mountiny bad news, we can't use solution 2, in very tight screens, this UI bug will happen
It happens because we made the Workspace type + the action in one column. I think it's better to keep the 4th column with a max width equal to 100px. What do you see? below a screenshot from the solution with 100 and in the same tight screen:
|
|
Ah alright, that I guess makes sense. I dont think we should be doing some major styling changes in cherry pick PR, so since as you mentioned this fragile pixel logic existed before, I think we could just update it for now to fix the styling in staging, but follow up with exploration on how to make this styling responsive without the esoteric pixel math |
|
@cubuspl42 you can review the code, please. |
|
Asking for a C+ who is available now |
Reviewer Checklist
Screenshots/VideosAndroid: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop
|
|
BUG Invite badge overlapping with workspace name on small screens 
|
|
Same is happening on iOS safari: 
|
|
Fixing now.. |
|
@allroundexperts fixed
|
|
@dragnoir Found another bug. The types are not aligned. 
|
|
@allroundexperts fixed
|
|
I will wait for Monday morning for @shawnborton to review but since this is a deploy blocker I would merge it past 10am CET to unblock deploy |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
|
Based on NewDot, Shawn is in the US so to make sure we can deploy sooner than later, I am going to go ahead with the merge here, I have tested as well and it looks good (do not worry about the blinking that is my local thing) |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
[CP Staging] Fix: align badge in workspace list (cherry picked from commit 0f30b3d)
|
🚀 Cherry-picked to staging by https://github.com/Beamanator in version: 1.4.63-18 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
|
Cool, I think that looks good. Only minor comment is that the overflow icon (three dots) on mobile feels like it's not quite as far to the right as it should be? I should have caught this sooner but it's strange that we're wrapping the overflow icon in a 40x40 wrapper and then sending that wrapper to the edge of the view. I think that's what's causing the weirdness above: Ideally the icon should just be 20x20 and there should be even 20px padding both vertically and horizontally on the entire row: None of that was caused by @dragnoir - but just noting in case we want to do a follow up PR to clean that stuff up. |
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.4.63-21 🚀
|
Details
This PR fixes the misalignment of the badge inside a row in workspace list.
Fixed Issues
$ #40524
PROPOSAL: #40524 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop