[Core Branding] Fix Compose Bar Text Alignment #13645
Conversation
|
Tested on web but looks like we are off by just a teeny bit: |
|
What are your thoughts on trying to get the little right border of the plus icon to line up with the right edge of the avatar above? |
|
How do you get the pixel overlay? Super helpful! I added 1px to the right margin |
|
Oop just saw your comment, I agree with lining up the divider line with the edge of the avatar |
|
Just so I'm clear, we want the right edge of the divider line to line up with the blue line, and then the left edge of the text (not including the '|' indicator) to align along the red line? |
|
|
grgia
changed the title
|
Nice, this is looking pretty good! For even more alignment, I think we can make the "+" button it's own button and make it 32x32 (like the send button). Then make it have a margin right of 3px and then the border line comes right after that. This way the + is perfectly centered with the avatars above it. Something like this (added red to make it super obvious):
Thoughts? |
|
One thing I am noticing too is that we might need to add 1 or 2 more px to the roundness of the overall compose bar. From the screenshots, it looks like it doesn't quite make a perfect pill shape: |
|
Yes, I agree with both of these suggestions! I'll get these in. One clarification- if we make the "+" button similar to the send button, the circular border (red in your pic) would still be transparent right? |
|
That's correct, let's still keep it transparent. |
|
Here's an update! |
|
Should I align the center of the expand icon with the plus or does this look good? |
|
This is looking good! I like the idea of center aligning the expand icon. So then the remaining item is the border radius of the compose box. I didn't realize it was actually 42px tall: And not 40px, which is what I think I would have expected it to be. We could either adjust it so that it rests at 40px, or just bump up the border radius to at least 21px. My preference would be to have it at 40px because that is a standard UI component size we use elsewhere, like the size of avatars. Thoughts? |
|
One easy approach is to just make sure the child elements inside the compose box wrapper are 38px tall and not 40px tall. So that this way we account for the 2px worth of border (1px top, 1px bottom, etc) we have. |
|
40px height + aligned expand I think because the compose bar is made up of three pieces, changing border radius right now from 0 to 100 looks like this: Screen.Recording.2022-12-20.at.7.33.31.PM.mov |
|
Nice, thanks for handling those. I think the issue with making the border radius anything greater than exactly half of the resting point of the compose bar height is that the corners will look too rounded when we expand the compose box. So, hence why we want 20px and hence why 20px wasn't quite cutting it when the compose box was actually 42px. So yeah, if we can get the compose box to start at 40px and then use 20px for border radius, we should be good to go. |
|
That makes sense! Here's where I left off last night |
|
Nice! Can you code inspect to see the height of the outermost wrapper of the compose box? The wrapper that we apply the border to. |
|
|
|
Lovely, looks great! Do you have the latest version of main merged in? My eagle eye tells me that's the old version of the send icon hehe |
|
Updated screenshots with 12px
|
melvin-bot
bot
requested review from
jasperhuangg and
sobitneupane
and removed request for
a team
|
@sobitneupane @jasperhuangg One of you needs to 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] |
| @@ -637,7 +639,7 @@ class ReportActionCompose extends React.Component { | |||
| </> | |||
| )} | |||
| </AttachmentPicker> | |||
| <View style={styles.textInputComposeSpacing}> | |||
| <View style={[styles.textInputComposeSpacing, styles.pl6]}> | |||
There was a problem hiding this comment.
Let's add the paddingLeft styles directly to textInputComposeSpacing.
src/styles/styles.js
Outdated
| pl6: { | ||
| paddingLeft: 6, | ||
| }, | ||
|
|
There was a problem hiding this comment.
Don't really need these if we follow my suggestion above, also since the paddingLeft styles aren't being used anywhere else.
There was a problem hiding this comment.
I'm also curious how this one might work with utilities/spacing.js in that the number after the pl typically denotes a multiplier which multiplies our default spacing unit of four:
There was a problem hiding this comment.
yeah, using those naming conventions pl6 definitely isn't accurate here, it doesn't mean paddingLeft: 6px rather paddingLeft: 24px, which is why I also wanted to get rid of that class
src/styles/styles.js
Outdated
| @@ -1739,13 +1750,13 @@ const styles = { | |||
| }, | |||
|
|
|||
| emptyAvatar: { | |||
| marginRight: variables.componentSizeNormal - 24, | |||
| marginRight: variables.componentSizeNormal - 26, | |||
There was a problem hiding this comment.
Any chance we could add a comment explaining where 26 is coming from?
src/styles/styles.js
Outdated
| height: variables.avatarSizeNormal, | ||
| width: variables.avatarSizeNormal, | ||
| }, | ||
|
|
||
| emptyAvatarSmall: { | ||
| marginRight: variables.componentSizeNormal - 28, | ||
| marginRight: variables.componentSizeNormal - 30, |
There was a problem hiding this comment.
Same, where is 30 coming from?
There was a problem hiding this comment.
Go to LHN, verify the space between avatar and username is 12px
Go to a Chat
Verify the space between avatar and username in header is 12px
Verify the space between avatar and chat is 12px
@grgia Space between avatar and username is 14px in all the above cases.
|
Everything should be 12px now! Also, I removed that padding variable and moved dependencies into the variables folder. Should be good for re-review!
|
There was a problem hiding this comment.
Reviewer Checklist
- I have verified the author checklist is complete (all boxes are checked off).
- I verified the correct issue is linked in the
### Fixed Issuessection above - I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the
Testssection - I verified the steps for Staging and/or Production testing are in the
QA stepssection - I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
- I verified the steps for local testing are in the
- I checked that screenshots or videos are included for tests on all platforms
- I included screenshots or videos for tests on all platforms
- I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
- If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
- I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e.
toggleReportand notonIconClick). - I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to
src/languages/*files and using the translation method - I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the
Waiting for Copylabel for a copy review on the original GH to get the correct copy. - I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in
STYLE.md) were followed
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e.
- If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
- I verified that this PR follows the guidelines as stated in the Review Guidelines
- I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like
Avatar, I verified the components usingAvatarhave been tested & I retested again) - I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
- I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
- If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a
/** comment above it */ - The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to
thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor) - Any internal methods bound to
thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick) - All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
- If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
- If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e.
StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
- If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like
Avataris modified, I verified thatAvataris working as expected in all cases) - If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
- If a new page is added, I verified it's using the
ScrollViewcomponent to make it scrollable when more elements are added to the page. - I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.
|
Looks great to me! I smacked the "Ready to build" label thinking it might trigger some testable builds? Will keep an eye there. Otherwise feel free to merge this one. |
|
I'm gonna go ahead and merge this one, not sure why the builds didn't work but 🤷 |
Performance Comparison Report 📊Significant Changes To DurationThere are no entries Meaningless Changes To DurationShow entries
Show details
|
|
🚀 Deployed to staging by @shawnborton in version: 1.2.54-1 🚀
|
|
🚀 Deployed to production by @AndrewGable in version: 1.2.54-2 🚀
|
Fixes alignment of compose text to match this photo:
Current Branch:
Details
Fixed Issues
$ #13595
$ #14006
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.Screenshots/Videos
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android