Skip to content
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

Fixed count, error state and created empty state component for scheduled post and draft #8626

Open
wants to merge 15 commits into
base: scheduled-post-options
Choose a base branch
from

Conversation

Rajat-Dabade
Copy link
Contributor

Summary

Ticket Link

Checklist

  • Added or updated unit tests (required for all new features)
  • Has UI changes
  • Includes text changes and localization file updates
  • Have tested against the 5 core themes to ensure consistency between them.
  • Have run E2E tests by adding label E2E iOS tests for PR.

Device Information

This PR was tested on:

Screenshots

Release Note


@Rajat-Dabade Rajat-Dabade marked this pull request as ready for review February 24, 2025 08:30
@Rajat-Dabade Rajat-Dabade self-assigned this Feb 24, 2025
@Rajat-Dabade Rajat-Dabade added the 2: Dev Review Requires review by a core commiter label Feb 24, 2025
const preferences = queryDisplayNamePreferences(database).
observeWithColumns(['value']);
const isMilitaryTime = preferences.pipe(map((prefs) => getDisplayNamePreferenceAsBool(prefs, 'use_military_time')));

// TODO: fix the count for DM's and GM's and Thread
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you create a Jira ticket to track this is planing to do it later?

const preferences = queryDisplayNamePreferences(database).
observeWithColumns(['value']);
const isMilitaryTime = preferences.pipe(map((prefs) => getDisplayNamePreferenceAsBool(prefs, 'use_military_time')));

// TODO: fix the count for DM's and GM's and Thread
const scheduledPostCount = currentTeamId.pipe(switchMap((teamId) => observeScheduledPostCountForChannel(database, teamId, channelId)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. We need to include thread ID as well (root post's ID) as when in a thread, we need to show only that thread's count. Similarly when in channels, we need to exclude thread scheduled posts from the count.

  2. You also need to fetch the scheduled post if count is 1, and replace the hardcoded value in the component where there's a todo //TODO: remove this hardcoded value with actual value with the scheduled post's scheduled at time. We need to display that time if there is only a single scheduled post in channel/thread.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only when crt is enabled, correct?

Copy link
Contributor Author

@Rajat-Dabade Rajat-Dabade Feb 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, added crt checks as well.

scheduledPostCount = observeScheduledPostCountForDMsAndGMs(database, channelId, isCRTEnabled);
} else if (channelType === 'O' && channelId) {
scheduledPostCount = currentTeamId.pipe(switchMap((teamId) => observeScheduledPostCountForChannel(database, teamId, channelId, isCRTEnabled)));
} else if (rootId) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If its a thread in open channel, the code will go the the above else if, not this else, right? That looks wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have reverted the code, Now we will be checking firstly if rootId is present or not, if present that means we have to extract the count of the thread. If not then extract the count of the channel, DMs and GMs.

rootId?: string;
}

const enhance = withObservables(['channelId', 'channelType', 'isCRTEnabled', 'rootId'], ({database, channelId, channelType, isCRTEnabled, rootId}: Props) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we expect this props to change over time while the component is mounted? if not, is ok to leave as `withObservables([], ){...}: Props) => {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, when the component is mounted, these props are not expected to change over time. So we can go with your suggestion.

Comment on lines 14 to 18
type Props = WithDatabaseArgs & {
channelId: string;
}

const enchanced = withObservables(['channelId'], ({database}: Props) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you need channelId here if is not being used at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed it. Will remove it. Nice catch 🙌

@@ -54,9 +53,16 @@ type Props = {
isMilitaryTime: boolean;
isThread?: boolean;
scheduledPostCount?: number;
channelId: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does not seem to be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, will remove it.

dateTime,
}}
<ScheduledPostIndicatorWithDatetime
channelId={channelId}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this component is not really using the channelId

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will remove it.

channelId: string,
isCRTEnabled: boolean,
) => {
let query = database.collections.get<ScheduledPostModel>(SCHEDULED_POST).query(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let query = database.collections.get<ScheduledPostModel>(SCHEDULED_POST).query(
let query = database.get<ScheduledPostModel>(SCHEDULED_POST).query(

@@ -29,6 +30,7 @@ const enhanced = withObservables([], ({database, serverUrl}: EnhanceProps) => {
const currentUserId = observeCurrentUserId(database);
const hasGMasDMFeature = observeHasGMasDMFeature(database);
const isBookmarksEnabled = observeConfigBooleanValue(database, 'FeatureFlagChannelBookmarks');
const isCRTEnabled = observeIsCRTEnabled(database);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need this here? I think that can be moved to the index file of SchedulePostIndicator

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

@@ -40,6 +40,7 @@ type ChannelProps = {
channelType: ChannelType;
hasGMasDMFeature: boolean;
includeBookmarkBar?: boolean;
isCRTEnabled: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move to the component that uses it, no need to have it here

const scheduledPostHasError = of(false);
const scheduledPostHasError = allScheduledPost.pipe(
// eslint-disable-next-line max-nested-callbacks
switchMap((scheduledPosts) => of(scheduledPosts.some((post) => post.errorCode !== ''))),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can extract this function scheduledPosts.some((post) => post.errorCode !== '') and get rid of the eslint-disable-line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

<ScheduledPostIndicator
isCRTEnabled={isCRTEnabled}
rootId={rootId}
channelId={EMPTY_STRING}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this makes me thing that channelId in SchedulePostIndicator should be optional

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -40,6 +41,8 @@ const styles = StyleSheet.create({
flex: {flex: 1},
});

const EMPTY_STRING = '';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very weird.. make channelId optional in the component that is receiving this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already made it optional; I forgot about these changes. Done it, corrected it.

@Rajat-Dabade Rajat-Dabade force-pushed the empty-state-scheduled-post branch from a036a49 to b83266f Compare March 11, 2025 10:18
@Rajat-Dabade Rajat-Dabade requested a review from enahum March 11, 2025 11:46
@Rajat-Dabade Rajat-Dabade force-pushed the empty-state-scheduled-post branch from d48ca4f to 218d3fd Compare March 12, 2025 13:17
const currentUser = observeCurrentUser(database);
const currentChannelId = observeCurrentChannelId(database);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why you need the current channel id for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed it.

@@ -15,11 +17,12 @@ import type {WithDatabaseArgs} from '@typings/database/database';
const enchanced = withObservables([], ({database}: WithDatabaseArgs) => {
const currentTeamId = observeCurrentTeamId(database);
const draftsCount = currentTeamId.pipe(switchMap((teamId) => observeDraftCount(database, teamId))); // Observe draft count
const scheduledPostCount = currentTeamId.pipe(switchMap((teamId) => observeScheduledPostCount(database, teamId, true))); // Observe scheduled post count
const allScheduledPost = currentTeamId.pipe(switchMap((teamId) => observeScheduledPostsForTeam(database, teamId, true))); // Observe scheduled post count
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment is misleading

@Rajat-Dabade Rajat-Dabade requested a review from enahum March 13, 2025 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2: Dev Review Requires review by a core commiter release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants