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

Issue 6390 - Fixes emoji positioning #6766

Merged
merged 3 commits into from
Dec 16, 2021
Merged

Issue 6390 - Fixes emoji positioning #6766

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
595f6c7
Emoji fixes on iOS - optionAlternateText
sidferreira Dec 15, 2021
ee5b660
Emoji fixes on iOS - Inputs
sidferreira Dec 15, 2021
8925786
Revert "Emoji fixes on iOS - Inputs"
sidferreira Dec 15, 2021
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
2 changes: 1 addition & 1 deletion src/pages/ReportDetailsPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ class ReportDetailsPage extends Component {
styles.sidebarLinkText,
styles.optionAlternateText,
styles.textLabelSupporting,
styles.mb6,
styles.mb5,
]}
numberOfLines={1}
>
Expand Down
3 changes: 1 addition & 2 deletions src/pages/home/HeaderView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ const HeaderView = (props) => {
>
<Pressable
onPress={() => {
if (ReportUtils.isDefaultRoom(props.report)) {
if (isDefaultChatRoom) {
return Navigation.navigate(ROUTES.getReportDetailsRoute(props.report.reportID));
}
if (participants.length === 1) {
Expand Down Expand Up @@ -154,7 +154,6 @@ const HeaderView = (props) => {
styles.sidebarLinkText,
styles.optionAlternateText,
styles.textLabelSupporting,
styles.mt1,
]}
numberOfLines={1}
>
Expand Down
2 changes: 1 addition & 1 deletion src/pages/home/sidebar/OptionRow.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ const OptionRow = (props) => {
: [styles.optionDisplayName, ...textUnreadStyle];
const alternateTextStyle = props.mode === 'compact'
? [textStyle, styles.optionAlternateText, styles.textLabelSupporting, styles.optionAlternateTextCompact]
: [textStyle, styles.optionAlternateText, styles.textLabelSupporting, styles.mt1];
: [textStyle, styles.optionAlternateText, styles.textLabelSupporting];
const contentContainerStyles = props.mode === 'compact'
? [styles.flex1, styles.flexRow, styles.overflowHidden, styles.alignItemsCenter]
: [styles.flex1];
Expand Down
2 changes: 0 additions & 2 deletions src/styles/optionAlternateTextPlatformStyles/index.ios.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
export default {
height: 20,
lineHeight: 20,
paddingTop: 1,
};
4 changes: 2 additions & 2 deletions src/styles/styles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1111,8 +1111,8 @@ const styles = {
},

optionAlternateText: {
height: 16,
lineHeight: 16,
height: 20,
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 not sure how I feel about this change. Right now, we have the user name/email set to be 20px tall, then a 4px margin, then the alternate text is 16px. This gives us 20 + 4 + 16 = 40px, which sits perfectly next to the 40px tall avatar. Can you make sure we get this same kind of evenness with these proposed changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shawnborton the margin was removed, as discussed in #6390 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

@sidferreira could you please expand on Shawn's comment. We want to be sure from a design perspective.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Quoting from the link:

To fix this, we need to tweak optionAlternateText in 3 places:

Use case 1

App/src/pages/home/HeaderView.js

Lines 151 to 159 in ecce012

<ExpensifyText
style={[
styles.sidebarLinkText,
styles.optionAlternateText,
styles.textLabelSupporting,
styles.mt1,
]}
numberOfLines={1}
>

Use case 2

App/src/pages/home/sidebar/OptionRow.js

Line 95 in 0311b18

: [textStyle, styles.optionAlternateText, styles.textLabelSupporting, styles.mt1];

Have a total height 20 as the component height is 16 and mt1 is 4.

Use case 3

App/src/pages/ReportDetailsPage.js

Lines 147 to 155 in ecce012

<ExpensifyText
style={[
styles.sidebarLinkText,
styles.optionAlternateText,
styles.textLabelSupporting,
styles.mb6,
]}
numberOfLines={1}
>

Has a total height 40 as the component height is 16 and mt6 is 24

So, my suggestion is to:
A) change optionAlternateText to have lineHeight/height: 20, behaving kinda like mt1 by default
B) Remove mt1 from use cases 1 and 2
C) Change mb6 to mb5 in use case 3

This change will make sure that all items keep their designed heights but allow space to emoji wherever is needed.

lineHeight: 20,
},

optionAlternateTextCompact: {
Expand Down