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.

Sign up for GitHub

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

fix display of undefined on card name after page refresh #52301

Merged
merged 5 commits into from
Nov 12, 2024
Merged

fix display of undefined on card name after page refresh #52301

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
82ebccf
fix display of undefined on card name after page refresh
huult Nov 11, 2024
487bc09
Add type parameter for company card setting feed name
huult Nov 11, 2024
ae40ea5
pass selected feed name when navigating to Settings Page
huult Nov 11, 2024
e67364f
Merge remote-tracking branch 'upstream/main' into 52140-undefined-car…
huult Nov 12, 2024
09f31d5
Add loading to load Onyx value
huult Nov 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/ROUTES.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1238,8 +1238,8 @@ const ROUTES = {
getRoute: (policyID: string) => `settings/workspaces/${policyID}/company-cards/settings` as const,
},
WORKSPACE_COMPANY_CARDS_SETTINGS_FEED_NAME: {
route: 'settings/workspaces/:policyID/company-cards/settings/feed-name',
getRoute: (policyID: string) => `settings/workspaces/${policyID}/company-cards/settings/feed-name` as const,
route: 'settings/workspaces/:policyID/company-cards/settings/feed-name/:feedName',
getRoute: (policyID: string, feedName: string) => `settings/workspaces/${policyID}/company-cards/settings/feed-name/${encodeURIComponent(feedName)}` as const,
},
WORKSPACE_RULES: {
route: 'settings/workspaces/:policyID/rules',
Expand Down
1 change: 1 addition & 0 deletions src/libs/Navigation/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -854,6 +854,7 @@ type SettingsNavigatorParamList = {
};
[SCREENS.WORKSPACE.COMPANY_CARDS_SETTINGS_FEED_NAME]: {
policyID: string;
feedName?: string;
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
feedName?: string;
selectedFeed: string;

Copy link
Contributor

Choose a reason for hiding this comment

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

Please update in other places

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DylanDylann We can't use selectedFeed because it will make things more complex. If you want to use selectedFeed, we must also send the cardFeeds object in the parameters.

Copy link
Contributor Author

@huult huult Nov 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If we use selectedFeed without passing the cardFeeds object, we encounter an issue like the video below because cardFeeds is retrieved from Onyx.

VIDEO
Screen.Recording.2024-11-11.at.15.30.07.mov

Copy link
Contributor

Choose a reason for hiding this comment

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

In src/pages/workspace/companyCards/WorkspaceCompanyCardsSettingsFeedNamePage.tsx, we can get cardFeed from Onyx, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

small note: Never give a video without any description and say it is a bug like in the video. When elaborating on a bug please say detailed steps, actual behavior, and expected behavior or at least a description to other people can understand that you want to say

we encounter an issue like the video below because cardFeeds is retrieved from Onyx.

Copy link
Contributor

@DylanDylann DylanDylann Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Are you aware of this, and if so, do you have any suggestions for handling it?

It seems I handled it in my ref branch. Did you check it? Or is there any problem?

Copy link
Contributor

Choose a reason for hiding this comment

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

we won’t need to set additional parameters.

Also fine, if that we need to handle loading indicator if lastSelectedFeed isn't loaded

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. Will update. thank you

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you test this case on your branch?

@huult It worked. Is there any problem?

Screen.Recording.2024-11-12.at.12.06.11.mov

};
[SCREENS.WORKSPACE.EXPENSIFY_CARD_DETAILS]: {
policyID: string;
Expand Down
6 changes: 4 additions & 2 deletions src/pages/workspace/companyCards/WorkspaceCompanyCardsSettingsFeedNamePage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ type WorkspaceCompanyCardsSettingsFeedNamePageProps = StackScreenProps<SettingsN

function WorkspaceCompanyCardsSettingsFeedNamePage({
route: {
params: {policyID},
params: {policyID, feedName: selectedFeedName},
},
}: WorkspaceCompanyCardsSettingsFeedNamePageProps) {
const styles = useThemeStyles();
Expand All @@ -41,7 +41,9 @@ function WorkspaceCompanyCardsSettingsFeedNamePage({
const [lastSelectedFeed] = useOnyx(`${ONYXKEYS.COLLECTION.LAST_SELECTED_FEED}${policyID}`);
const [cardFeeds] = useOnyx(`${ONYXKEYS.COLLECTION.SHARED_NVP_PRIVATE_DOMAIN_MEMBER}${workspaceAccountID}`);
const selectedFeed = CardUtils.getSelectedFeed(lastSelectedFeed, cardFeeds);
const feedName = cardFeeds?.settings?.companyCardNicknames?.[selectedFeed] ?? translate('workspace.companyCards.feedName', {feedName: CardUtils.getCardFeedName(selectedFeed)});
const feedName = selectedFeed
? cardFeeds?.settings?.companyCardNicknames?.[selectedFeed] ?? translate('workspace.companyCards.feedName', {feedName: CardUtils.getCardFeedName(selectedFeed)})
: decodeURIComponent(selectedFeedName ?? '');

const validate = useCallback(
(values: FormOnyxValues<typeof ONYXKEYS.FORMS.WORKSPACE_COMPANY_CARD_FEED_NAME>) => {
Expand Down
2 changes: 1 addition & 1 deletion src/pages/workspace/companyCards/WorkspaceCompanyCardsSettingsPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ function WorkspaceCompanyCardsSettingsPage({
const isPersonal = liabilityType === CONST.COMPANY_CARDS.DELETE_TRANSACTIONS.ALLOW;

const navigateToChangeFeedName = () => {
Navigation.navigate(ROUTES.WORKSPACE_COMPANY_CARDS_SETTINGS_FEED_NAME.getRoute(policyID));
Navigation.navigate(ROUTES.WORKSPACE_COMPANY_CARDS_SETTINGS_FEED_NAME.getRoute(policyID, feedName));
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
Navigation.navigate(ROUTES.WORKSPACE_COMPANY_CARDS_SETTINGS_FEED_NAME.getRoute(policyID, feedName));
Navigation.navigate(ROUTES.WORKSPACE_COMPANY_CARDS_SETTINGS_FEED_NAME.getRoute(policyID, selectedFeed));

};

const deleteCompanyCardFeed = () => {
Expand Down
Loading