Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Core Branding] Fix Compose Bar Text Alignment #13645

Merged
merged 14 commits into from
Jan 13, 2023
Merged

[Core Branding] Fix Compose Bar Text Alignment #13645

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
17fa2ba
Align compose bar text with sent text
grgia Dec 15, 2022
64b03da
Add 1px to right margin
grgia Dec 16, 2022
0f4d0cc
Merge branch 'main' into georgia-composeFix
grgia Dec 16, 2022
6fa4d92
Align divider with edge of avatar, Text with username
grgia Dec 16, 2022
9a84771
Align plus/expand in compose bar, change compose height to 40px
grgia Dec 21, 2022
103f632
Make composer total height 40px
grgia Dec 21, 2022
a288943
Merge branch 'main' into georgia-composeFix
grgia Dec 22, 2022
0db6611
Close View component
grgia Dec 22, 2022
00c6ba0
Merge branch 'main' into georgia-composeFix
grgia Jan 9, 2023
d88a71d
Move reused constant into variables
grgia Jan 9, 2023
c3a9534
Standardize avatar right margin to 12px in LHN, chats, etc.
grgia Jan 10, 2023
d93697b
Align text after avatar
grgia Jan 10, 2023
4cb7201
Fix "Write something..." padding
grgia Jan 10, 2023
32a682c
Use variables to control 12px spacing between avatar and chats/names
grgia Jan 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 17 additions & 15 deletions src/pages/home/report/ReportActionCompose.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -600,20 +600,22 @@ class ReportActionCompose extends React.Component {
</Tooltip>
)}
<Tooltip text={this.props.translate('reportActionCompose.addAction')}>
<TouchableOpacity
ref={el => this.actionButton = el}
onPress={(e) => {
e.preventDefault();

// Drop focus to avoid blue focus ring.
this.actionButton.blur();
this.setMenuVisibility(true);
}}
style={styles.chatItemAttachButton}
disabled={isBlockedFromConcierge || this.props.disabled}
>
<Icon src={Expensicons.Plus} />
</TouchableOpacity>
<View style={styles.chatItemAttachBorder}>
<TouchableOpacity
ref={el => this.actionButton = el}
onPress={(e) => {
e.preventDefault();

// Drop focus to avoid blue focus ring.
this.actionButton.blur();
this.setMenuVisibility(true);
}}
style={styles.chatItemAttachButton}
disabled={isBlockedFromConcierge || this.props.disabled}
>
<Icon src={Expensicons.Plus} />
</TouchableOpacity>
</View>
</Tooltip>
</View>
<PopoverMenu
Expand All @@ -637,7 +639,7 @@ class ReportActionCompose extends React.Component {
</>
)}
</AttachmentPicker>
<View style={styles.textInputComposeSpacing}>
<View style={[styles.textInputComposeSpacing, styles.pl6]}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add the paddingLeft styles directly to textInputComposeSpacing.

<DragAndDrop
dropZoneId={CONST.REPORT.DROP_NATIVE_ID}
activeDropZoneId={CONST.REPORT.ACTIVE_DROP_NATIVE_ID}
Expand Down
41 changes: 26 additions & 15 deletions src/styles/styles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,10 @@ const styles = {
paddingLeft: 0,
},

pl6: {
paddingLeft: 6,
},

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't really need these if we follow my suggestion above, also since the paddingLeft styles aren't being used anywhere else.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

textDanger: {
color: themeColors.danger,
},
Expand Down Expand Up @@ -691,11 +695,11 @@ const styles = {
},

chatItemComposeSecondaryRowOffset: {
marginLeft: 48,
marginLeft: variables.chatInputSpacing,
},

offlineIndicator: {
marginLeft: 48,
marginLeft: variables.chatInputSpacing,
},

offlineIndicatorMobile: {
Expand All @@ -710,6 +714,7 @@ const styles = {
// Actions
actionAvatar: {
borderRadius: 20,
marginRight: 6,
},

componentHeightLarge: {
Expand Down Expand Up @@ -1306,7 +1311,7 @@ const styles = {
flexShrink: 1,
flexBasis: 0,
position: 'relative',
marginLeft: 48,
marginLeft: variables.chatInputSpacing,
},

chatItemRight: {
Expand Down Expand Up @@ -1451,7 +1456,7 @@ const styles = {
backgroundColor: themeColors.transparent,
height: 32,
padding: 6,
margin: 4,
margin: 3,
justifyContent: 'center',
},

Expand Down Expand Up @@ -1525,8 +1530,7 @@ const styles = {
alignSelf: 'flex-end',
borderRadius: variables.buttonBorderRadius,
height: 32,
marginVertical: 4,
marginLeft: 3,
marginVertical: 3,
paddingHorizontal: 6,
justifyContent: 'center',
},
Expand All @@ -1540,24 +1544,31 @@ const styles = {
},

chatItemAttachButton: {
alignItems: 'center',
alignSelf: 'flex-end',
borderRightColor: themeColors.border,
borderRightWidth: 1,
borderRadius: variables.componentBorderRadiusRounded,
backgroundColor: themeColors.transparent,
height: 32,
width: 32,
marginBottom: 4,
marginTop: 4,
marginLeft: 4,
padding: 6,
marginLeft: 3,
marginRight: 3,
justifyContent: 'center',
},

chatItemAttachBorder: {
borderRightColor: themeColors.border,
borderRightWidth: 1,
marginBottom: 3,
marginTop: 3,

},

composerSizeButton: {
alignItems: 'center',
alignSelf: 'flex-end',
height: 26,
marginBottom: 6,
marginTop: 6,
marginRight: 4,
justifyContent: 'center',
width: 32,
},
Expand Down Expand Up @@ -1739,13 +1750,13 @@ const styles = {
},

emptyAvatar: {
marginRight: variables.componentSizeNormal - 24,
marginRight: variables.componentSizeNormal - 26,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any chance we could add a comment explaining where 26 is coming from?

height: variables.avatarSizeNormal,
width: variables.avatarSizeNormal,
},

emptyAvatarSmall: {
marginRight: variables.componentSizeNormal - 28,
marginRight: variables.componentSizeNormal - 30,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same, where is 30 coming from?

height: variables.avatarSizeSmall,
width: variables.avatarSizeSmall,
},
Expand Down
3 changes: 2 additions & 1 deletion src/styles/variables.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ export default {
contentHeaderHeight: getValueUsingPixelRatio(65, 100),
componentSizeSmall: getValueUsingPixelRatio(28, 32),
componentSizeNormal: 40,
inputComponentSizeNormal: 42,
inputComponentSizeNormal: 40,
componentSizeLarge: 52,
componentBorderRadius: 8,
componentBorderRadiusSmall: 4,
Expand Down Expand Up @@ -84,4 +84,5 @@ export default {
sliderBarHeight: 8,
sliderKnobSize: 26,
checkboxLabelActiveOpacity: 0.7,
chatInputSpacing: 54,
};