-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
List of Scheduled post in Scheduled post tab #8602
base: add-create-at-column-scheduled-post
Are you sure you want to change the base?
List of Scheduled post in Scheduled post tab #8602
Conversation
6a284f4
to
68eff02
Compare
de2b148
to
3a6f367
Compare
const deleteDraft = useCallback(() => { | ||
const deletePost = useCallback(() => { | ||
if (draftType === DRAFT_TYPE_SCHEDULED) { | ||
// TODO: Add swipeable delete for schedule post |
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 will be handled in the subsequent PR. https://mattermost.atlassian.net/browse/MM-62200
maxToRenderPerBatch={10} | ||
nativeID={location} | ||
renderItem={renderItem} | ||
ListEmptyComponent={DraftEmptyComponent} //TODO: Change this to ScheduledPostEmptyComponent |
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.
Will handle this in subsequent PR. https://mattermost.atlassian.net/browse/MM-63142
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.
Some questions non-blocking
@@ -34,7 +34,7 @@ const getStyleSheet = (width: number) => { | |||
width, | |||
}, | |||
hiddenTabContent: { | |||
display: 'none', | |||
opacity: 0, |
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 this change?
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.
I recall an issue with rendering the Scheduled Post list in the tab for the first time. Since @harshilsharma63 owns this code, he might have more context on it.
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 is because the way we have tabs, we shift the tab contents left or right when navigating to a different tab. If you go from Tab A to Tab B, and you hide tab A by display: none
, now it no longer takes any width and the Tab B immediately and abrubtly shows up in the screen and there is no way to smoothen it. But, if you set opacity 0 of tab A, it still occupies the width and you can smoothly transition tab B into screen. This is how all tabs work. I had set display none mistakenly earlier and this corrects it.
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.
Please take a look to the date formatting issues. The rest is not so important from my side, but nice to haves.
app/screens/global_drafts/components/global_scheduled_post_list/global_scheduled_post_list.tsx
Show resolved
Hide resolved
app/screens/global_drafts/components/global_scheduled_post_list/global_scheduled_post_list.tsx
Outdated
Show resolved
Hide resolved
app/screens/global_drafts/components/global_scheduled_post_list/index.tsx
Show resolved
Hide resolved
app/screens/global_drafts/components/draft_and_scheduled_post_swipe_actions.tsx
Show resolved
Hide resolved
app/components/draft_scheduled_post_header/draft_scheduled_post_header.tsx
Show resolved
Hide resolved
8ee12ac
to
96f7359
Compare
68bf2da
to
63d31e5
Compare
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.
I am going to pre-approve, but please extract to constants the size of the tooltip.
export const DRAFT_TYPE_DRAFT = 'draft'; | ||
export const DRAFT_TYPE_SCHEDULED = 'scheduled'; |
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.
In a different PR I was commenting that if you add here as const
to both constants, you will probably have to use less castings.
tooltipContentStyle: { | ||
borderRadius: 8, | ||
width: 247, | ||
padding: 16, | ||
height: 160, | ||
}, |
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.
If it is constant across the platform, extract the constants to a common file, and use the constants.
I don't like it is constant (the container should accommodate the content) but I understand that is out of the scope right now.
const [layoutWidth, setLayoutWidth] = useState(0); | ||
const [tooltipVisible, setTooltipVisible] = useState(false); | ||
const onLayout = useCallback((e: LayoutChangeEvent) => { | ||
setLayoutWidth(e.nativeEvent.layout.width - DRAFT_SCHEDULED_POST_LAYOUT_PADDING); // 40 is the padding of the container |
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.
The comment is no longer needed, I think.
@@ -63,7 +63,7 @@ const GlobalDraftsList: React.FC<Props> = ({ | |||
const [tooltipVisible, setTooltipVisible] = useState(false); | |||
const onLayout = useCallback((e: LayoutChangeEvent) => { | |||
if (location === Screens.GLOBAL_DRAFTS) { | |||
setLayoutWidth(e.nativeEvent.layout.width - 40); // 40 is the padding of the container | |||
setLayoutWidth(e.nativeEvent.layout.width - DRAFT_SCHEDULED_POST_LAYOUT_PADDING); // 40 is the padding of the container |
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 comment here is no longer needed.
@Rajat-Dabade I don't see any changes since my last review. Did you miss a push? |
/>, | ||
); | ||
|
||
fireEvent.press(getByTestId('draft.tooltip.close.button')); |
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 should be inside an act, I think
Summary
Ticket Link
Checklist
E2E iOS tests for PR
.Device Information
This PR was tested on:
Screenshots
Release Note
Note: Please do the code review, I am parallelly adding unit tests.