Skip to content

No wrap style#7339

Merged
stitesExpensify merged 1 commit intomainfrom
chirag-adding-noWrap-style
Jan 20, 2022
Merged

No wrap style#7339
stitesExpensify merged 1 commit intomainfrom
chirag-adding-noWrap-style

Conversation

@chiragsalian
Copy link
Copy Markdown
Contributor

@chiragsalian chiragsalian commented Jan 20, 2022 •
edited
Loading

Details

Fixed Issues

$ #7321

Tests / QA

Tested the same steps mentioned on #7321 and confirmed the image has ellipses on mWeb.
image

  • Verify that no errors appear in the JS console

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

@chiragsalian chiragsalian self-assigned this Jan 20, 2022
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jan 20, 2022

⚠️ ⚠️ Heads up! This pull request has the CP Staging label. ⚠️ ⚠️
Merging it will cause it to be immediately deployed to staging, even if the open StagingDeployCash deploy checklist is locked.

@chiragsalian chiragsalian marked this pull request as ready for review January 20, 2022 20:10
@chiragsalian chiragsalian requested a review from a team as a code owner January 20, 2022 20:10
@MelvinBot MelvinBot requested review from Jag96 and removed request for a team January 20, 2022 20:11
roryabraham
roryabraham reviewed Jan 20, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

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

LGTM but not explicitly approving because I haven't tested.

stitesExpensify
stitesExpensify approved these changes Jan 20, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@stitesExpensify stitesExpensify left a comment

Choose a reason for hiding this comment

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

LGTM

@parasharrajat
Copy link
Copy Markdown
Member

parasharrajat commented Jan 20, 2022

Hey, I am here. Sorry for the delay.

If you want it to look like the old way then we should use numberOfLines={2} then it will start clipping from second line.

@stitesExpensify
Copy link
Copy Markdown
Contributor

stitesExpensify commented Jan 20, 2022

Does it look different this way? I'm not seeing it

@chiragsalian
Copy link
Copy Markdown
Contributor Author

chiragsalian commented Jan 20, 2022

I think the style change alone is fine for now. Plus i feel this keeps the UI consistent with mobile and web since i see a single line on them as well.

@parasharrajat
Copy link
Copy Markdown
Member

parasharrajat commented Jan 20, 2022

Previously, HeaderView was used to wrap to two lines and then clipped with an ellipsis. You can see that on PROD.

@stitesExpensify
Copy link
Copy Markdown
Contributor

stitesExpensify commented Jan 20, 2022

Gotcha. Well since we only see one line on web, I think this is fine 👍

@stitesExpensify stitesExpensify merged commit b069acd into main Jan 20, 2022
@stitesExpensify stitesExpensify deleted the chirag-adding-noWrap-style branch January 20, 2022 21:11
OSBotify pushed a commit that referenced this pull request Jan 20, 2022
@stitesExpensify @OSBotify
Merge pull request #7339 from Expensify/chirag-adding-noWrap-style
1e4bf1c 
No wrap style

(cherry picked from commit b069acd)
This was referenced Jan 20, 2022
Merged
Closed
@OSBotify
Copy link
Copy Markdown
Contributor

OSBotify commented Jan 20, 2022

🚀 Cherry-picked to staging by @stitesExpensify in version: 1.1.31-1 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

@OSBotify
Copy link
Copy Markdown
Contributor

OSBotify commented Jan 21, 2022

🚀 Deployed to production by @chiragsalian in version: 1.1.31-1 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify OSBotify mentioned this pull request Jan 24, 2022
Closed
33 tasks
@OSBotify OSBotify mentioned this pull request Aug 16, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@roryabraham roryabraham roryabraham left review comments

@stitesExpensify stitesExpensify stitesExpensify approved these changes

@Jag96 Jag96 Awaiting requested review from Jag96

Assignees

@chiragsalian chiragsalian

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@chiragsalian @parasharrajat @stitesExpensify @OSBotify @roryabraham