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

Add ui to switch between recipes and chefs feeds #1588

Merged
merged 3 commits into from
May 31, 2024

Conversation

Divs-B
Copy link
Contributor

@Divs-B Divs-B commented May 24, 2024

What's changed?

This will add radio buttons to switch between recipes and chefs feeds.
Based on selection the result will display accordingly.
At present data for both recipes and chefs are based on fixture data and will be replaced by live items soon.

ref: https://trello.com/c/GlwlhweJ/2935-add-ui-for-switching-between-different-searches
Note: Search functionality is not done in this PR, will be covered in next task.

Screen.Recording.2024-05-24.at.11.19.54.mov

Implementation notes

Checklist

General

  • 🤖 Relevant tests added
  • ✅ CI checks / tests run locally
  • 🔍 Checked on CODE

Client

  • 🚫 No obvious console errors on the client (i.e. React dev mode errors)
  • 🎛️ No regressions with existing user interactions (i.e. all existing buttons, inputs etc. work)
  • 📷 Screenshots / GIFs of relevant UI changes included

@Divs-B Divs-B requested a review from a team as a code owner May 24, 2024 10:21
Copy link
Contributor

@fredex42 fredex42 left a comment

Choose a reason for hiding this comment

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

Great work, though I think we should switch the "magic strings" for enum values before merging.

</React.Fragment>
);
}: Props) => {
const [selectedOption, setSelectedOption] = useState<string>('recipeFeed');
Copy link
Contributor

Choose a reason for hiding this comment

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

Tiny point - I've found that if you specify a default value like this, Typescript can infer the type and you don't have to specify it directly with <string>

Copy link
Contributor

Choose a reason for hiding this comment

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

My preference for this is to use numbers, defined as enums (https://www.typescriptlang.org/docs/handbook/enums.html); reason is that it's quicker for the machine to compare and process numbers than strings. However these days I guess that the performance difference is negligible though I would encourage the use of enums or constants over "magic value" strings because if you put a typo in an enum value the compiler complains wheras if you put a typo in a "magic value" string the app breaks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes more sense. updated with enum. thanks @fredex42

</InputContainer>
<RadioGroup>
<RadioButton
checked={selectedOption === 'recipeFeed'}
Copy link
Contributor

Choose a reason for hiding this comment

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

See my point above about defining 'recipeFeed' as an enum value

Comment on lines 78 to 85
{selectedOption === 'recipeFeed'
? Object.values(recipes).map((recipe) => (
<RecipeFeedItem key={recipe.id} recipe={recipe} />
))
: Object.values(chefs).map((chef) => (
<ChefFeedItem key={chef.id} chef={chef} />
))}
</FixedContentContainer>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that this is fine for the time being, but the ternary syntax doesn't extend very nicely if we want to put a third option in. To be honest my preference here would be to use a second functional component in the file which contains a switch block on the selectedOption value (passed over as a param) but I wouldn't bother making that change now unless we know that we are going to need a third case 😁

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah changed to switch case, thanks. This will help in scalability.

Copy link
Contributor

@jonathonherbert jonathonherbert left a comment

Choose a reason for hiding this comment

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

Looks great! 👏

@Divs-B Divs-B merged commit a697c95 into main May 31, 2024
9 checks passed
@Divs-B Divs-B deleted the db/add-search-ui-for-recipe-chef branch May 31, 2024 08:23
@prout-bot
Copy link

Seen on PROD (merged by @Divs-B 5 minutes and 52 seconds ago) Please check your changes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants