-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
feat: added conditional styles for chat focus mode #15319
Conversation
@chiragsalian @eVoloshchak 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] |
Reviewer Checklist
Screenshots/Videos |
@aneequeahmad Can you jump in here and explain the reason? |
@eVoloshchak I was reading the comments again and found out that we decided to ignore this small misalignment because its noticeable when zoomed only. #14148 (comment) |
That part was about it being slightly (1px) misaligned on web, not on native platforms @aneequeahmad, friendly bump on the above |
Native platforms currently are only slightly misaligned, it seems like there is no way to have them perfectly align |
@eVoloshchak The PR does include the fix for web. Do you want me to make any other change? Please let me know. |
Just delete the |
@eVoloshchak Removed the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
cc: @chiragsalian
This fixes the issue only on web/mWeb/Desktop, there seems to be no reliable fix for native
@eVoloshchak any changes required on this PR yet ? |
@aneequeahmad, do you have a fix that would make it centered on native platforms? |
I'm not sure why i was removed as the reviewer here so I added myself. Waiting to hear if there is a native platform fix by @aneequeahmad. |
@aneequeahmad, bump on the above |
@chiragsalian, it seems like there is no reliable fix for native. Should we fix this for web/Desktop only? |
Sure that is fine with me for now. If it becomes a pain point for native we can create a follow up GH issue to discuss and fix. @allroundexperts, you might want to specify to QA to test only on web/Desktop so that they don't waste time testing on mobile. |
@chiragsalian updated the steps. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/chiragsalian in version: 1.2.88-0 🚀
|
🚀 Deployed to staging by https://github.com/chiragsalian in version: 1.2.88-0 🚀
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 1.2.88-2 🚀
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 1.2.88-2 🚀
|
Details
This PR changes
alignItems
fromcenter
tobaseline
on web. This causes the channel name and the channel text to have the exact same baseline despite the difference in line heights / font sizes between the two texts. On native apps, this involves removing the line height from alternating text.Fixed Issues
$ #14148
PROPOSAL: #14148 (comment)
Tests
Note: This should be teated on Web and Desktop only.
Offline tests
QA Steps
Note that this should be tested on Web and Desktop only.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.Screenshots/Videos
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android