Skip to content

Updated the SaveSearchButton to indicate that the keyword is saved if…#124

Merged
NwinNwin merged 2 commits intomainfrom
122-shows-that-the-keyword-is-already-saved-in-savesearchbutton
Apr 9, 2025
Merged

Updated the SaveSearchButton to indicate that the keyword is saved if…#124
NwinNwin merged 2 commits intomainfrom
122-shows-that-the-keyword-is-already-saved-in-savesearchbutton

Conversation

@brelieu05
Copy link
Copy Markdown
Contributor

@brelieu05 brelieu05 commented Mar 17, 2025

… the user already saved it

Closes #122

image

@brelieu05 brelieu05 requested review from NwinNwin and tyleryy March 17, 2025 05:12
@brelieu05 brelieu05 linked an issue Mar 17, 2025 that may be closed by this pull request
@brelieu05 brelieu05 self-assigned this Mar 17, 2025
Copy link
Copy Markdown
Member

@tyleryy tyleryy left a comment

Choose a reason for hiding this comment

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

requested some minor changes.

) :
"Sorry, an error occurred while saving your search term. Please try again.",
status: success ? (description === "added" ? "success" : "info") : "error",
title: success
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

might be better to unnest the conditions from the toast objects, just so it is a little cleaner. Currently, it is toast -> API check -> description "added" check (where -> means nested conditional).

Maybe do API check -> toast -> description added?

if you do it this way, the fail api check should be the same i believe. and then the success api check will have two different toast jsons.

its kind of like making a more efficient tree. the new way is shallower (2 levels) and the current way is deeper (like 3 levels)


const addKeyword = async (keyword) => {
if (keywords.includes(keyword)) {
return { success: true, description: "existing" };
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

idk how else description is being used in this api response, but there seem to be only two states (existing and added). Maybe use a boolean?

@brelieu05
Copy link
Copy Markdown
Contributor Author

Sorry, i don't know why I didn't see the notification/email for this review weeks ago, so I just saw it now. Lmk if these changes reflects what you meant.

@brelieu05 brelieu05 requested a review from tyleryy April 7, 2025 00:13
Copy link
Copy Markdown
Member

@tyleryy tyleryy left a comment

Choose a reason for hiding this comment

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

Lgtm

@NwinNwin NwinNwin merged commit 7c6349f into main Apr 9, 2025
1 check passed
@NwinNwin NwinNwin deleted the 122-shows-that-the-keyword-is-already-saved-in-savesearchbutton branch April 9, 2025 04:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Shows that the keyword is already saved in SaveSearchButton

3 participants