-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[TS migration] Migrate 'MentionSuggestions.js' component to TypeScript #30514
[TS migration] Migrate 'MentionSuggestions.js' component to TypeScript #30514
Conversation
44c7007
to
4230a53
Compare
4230a53
to
52ff093
Compare
textUppercase: { | ||
textTransform: 'uppercase', | ||
}, | ||
|
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.
why did You remove this?
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.
After the removal of nonexistent styles.text
, the script findUnusedKeys
starts failing with these styles
we do not use these styles in the project
17077d7
to
4ed1744
Compare
I think you can remove [NO QA] from the title and try add some QA Steps, thanks! 😄 |
095510e
to
0138d94
Compare
@blazejkustra PR updated |
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.
Looks good, before making it ready for review someone from CK should review this Pr as well.
Ah also, don't forget to add the steps |
@kubabutkiewicz @blazejkustra PR updated |
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 😄
33476c2
to
eb58d70
Compare
Reviewer Checklist
Screenshots/VideosAndroid: Native30514.Android.mp4Android: mWeb Chrome30514.mWeb-Chrome.mp4iOS: Native30514.iOS.mp4iOS: mWeb Safari30514.mWeb-Safari.mp4MacOS: Chrome / Safari30514.Web.mp4MacOS: Desktop30514.Desktop.mp4 |
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 👍
We did not find an internal engineer to review this PR, trying to assign a random engineer to #25062 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
Co-authored-by: Monil Bhavsar <monilbhavsar25@gmail.com>
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.
@pasyukevich could you please update PR author checklist. Looks like it is updated and hence the check is failing
@MonilBhavsar I guess it is connected with this issue |
updated with the latest main and solved conflicts |
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.
Looks good, thanks!
✋ 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 production by https://github.com/mountiny in version: 1.4.0-3 🚀
|
Details
Fixed Issues
$ #25062
PROPOSAL:
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.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
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.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
native-android.mp4
Android: mWeb Chrome
web-android.mp4
iOS: Native
native-ios.mp4
iOS: mWeb Safari
web-ios.mp4
MacOS: Chrome / Safari
web.mp4
MacOS: Desktop
desktop.mp4