-
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
Clean up Thread UI #18876
Clean up Thread UI #18876
Conversation
@aimane-chnaif @MonilBhavsar 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] |
src/components/MultipleAvatars.js
Outdated
@@ -40,6 +40,9 @@ const propTypes = { | |||
|
|||
/** Whether #focus mode is on */ | |||
isFocusMode: PropTypes.bool, | |||
|
|||
/** Whether avatars are displayed on a report */ | |||
isInReportView: PropTypes.bool, |
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.
This prop name looks different than what it does. Can we rename this?
@@ -5,11 +5,14 @@ import _ from 'underscore'; | |||
import styles from '../../../styles/styles'; | |||
import * as Report from '../../../libs/actions/Report'; | |||
import withLocalize, {withLocalizePropTypes} from '../../../components/withLocalize'; | |||
import withWindowDimensions, {windowDimensionsPropTypes} from '../../../components/withWindowDimensions'; | |||
|
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.
NAB, empty line
@@ -24,43 +27,56 @@ const propTypes = { | |||
/** ID of child thread report */ | |||
childReportID: PropTypes.string.isRequired, | |||
|
|||
/** localization props */ | |||
/** Is hovered */ |
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.
Could you more detail. May be - "Is thread item/message hovered?"
style={[styles.ml2, styles.textMicroSupporting]} | ||
>{`${props.translate('threads.lastReply')} ${props.datetimeToCalendarTime(props.mostRecentReply)}`}</Text> | ||
const ReportActionItemThread = (props) => { | ||
const numberReplies = props.numberOfReplies > 99 ? '99+' : `${props.numberOfReplies}`; |
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.
This could be defined in CONST, something like MAX_THREAD_REPLIES_MESSAGE
src/languages/en.js
Outdated
today: 'Today', | ||
tomorrow: 'Tomorrow', | ||
yesterday: 'Yesterday', |
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.
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.
Agreed, pushed a fix!
Ready for another round! |
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.
BaseGenericPressable
change is just for prettier?
Please pull from main
. Now prettier diff is gone
@aimane-chnaif @MonilBhavsar I know it's late for y'all but if you're still around a final review of this would be great so that we can get it deployed in the next hour or so! |
reviewing in 30 min |
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.
Looking great! One small request, but it's NAB
Waiting for @aimane-chnaif review and then we will merge |
@aimane-chnaif have you had a chance to take a look yet? We want to get this merged ASAP |
checklisting right now |
Reviewer Checklist
Screenshots/VideosiOSios.mov |
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.
- Verify that no errors appear in the JS console
- test a thread with 100+ comments, ensure that # replies is '99+'
- QA that icons are 28x28 (or 32x32 with border)
- QA that small screens truncate date
Looks good, tests well!
NAB: It would be good to show tooltip like other subscript avatars Screen.Recording.2023-05-16.at.8.15.59.PM.mov |
Please add |
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
I updated the QA section to mention same as tests |
✋ 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.3.15-0 🚀
|
#18876 (comment) |
Issue #19142 is failing step 4 in Android only |
@aimane-chnaif Full details can be found in the issue #19142 - Here's the screenshot |
🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.3.15-12 🚀
|
Details
Fixed Issues
$ #18872
Tests
Offline tests
QA Steps
Same as Tests
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
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.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
Web
Screen.Recording.2023-05-15.at.7.16.02.PM.mov
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android