-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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.
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
Enlarge emojis in other contexts than just single character messages #47547
base: main
Are you sure you want to change the base?
Conversation
# Conflicts: # src/components/Composer/index.tsx
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
@parasharrajat 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] |
# Conflicts: # src/components/Composer/index.native.tsx # src/pages/home/report/comment/TextCommentFragment.tsx
Personally I think we should avoid that. I don't think we should be making too many conditional spacing adjustments in the UI if we can avoid it. cc @Expensify/design for thoughts too. |
Agree! I feel like this situation is not worth addressing. Nothing is broken, nothing looks absolutely terrible, and I personally feel like this is just one of those consequences of users doing things we have no way of predicting. If something looked really borked because of this, I'd say it's worth addressing, but it doesn't really feel that way to me. 🤷♂️ Just my 2 cents. |
@VickyStash Any updates. From the above comments, It seems that we can ignore that spacing issue. |
@parasharrajat All other issues are related to how react-native-live-markdown works.
Note: I'll be OOO 5th-6th Sept |
I would prefer it to be solved on lib then. Styles hacks do not scale well across platforms. |
Updates: after merging the latest main, a couple of weird issues appeared on iOS. For example: on the first report screen open the emojis in the display names and messages looks okay, but if I reopen the same screen it's overlapped. I'm working on the fixes. |
Thanks for the update. |
# Conflicts: # src/components/AccountSwitcher.tsx
I haven't had a lot of time for this issue today, but should be more focused on it tomorrow.
I've found the reason of this issues and was able to find a fix. It was related to the |
This two seems to be resolved with:
That's how it works: fixed_ios_2_prs.mp4 |
Both PRs to react-native-live-markdown have been merged: |
The react-native-live-markdown version will be bumped in this PR. As for this one:
@parasharrajat @tomekzaw
I achieved more expected alignment by manually applying to the
But it looks like to be able to handle it we need to add the support for the |
@VickyStash Let me summon @Skalakid who's the author of web implementation of react-native-live-markdown. @Skalakid Could you please respond to #47547 (comment)? |
Hi @VickyStash, I think we can just set the middle alignment for the text spans by default in |
# Conflicts: # src/components/Composer/index.tsx
@Skalakid It looks like setting the middle alignment by default in
But we can try to update
This way it affects only singleline inputs. What do you think? |
@VickyStash oh okay, I think it a good idea. Let's add the css style 😄 |
I've opened the PR for the web fix mentioned above: Expensify/react-native-live-markdown#486 |
Updates: Note: I'm going to be OOO next week (Sept 16-20) 🌴 |
Details
This is another try to merge the PR related to emojis enlarging, the previous one was reverted.
Fixed Issues
$ #4114
PROPOSAL: N/A
Tests
Composer:
Report history:
Settings:
Offline tests
Same as in the Tests section.
QA Steps
Same as in the Tests section.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop