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 routing #653

Merged
merged 5 commits into from
Jun 25, 2023
Merged

Add routing #653

merged 5 commits into from
Jun 25, 2023

Conversation

acrantel
Copy link
Member

@acrantel acrantel commented Jun 22, 2023

adds routing and EpisodeContext. also adds stubs for a lot of pages

@acrantel acrantel marked this pull request as draft June 22, 2023 02:23
@acrantel acrantel marked this pull request as ready for review June 25, 2023 03:38
Copy link
Contributor

@lowtorola lowtorola left a comment

Choose a reason for hiding this comment

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

This looks amazing!! I built it on my local and it looks so clean! Just had some questions that honestly we could discuss during our meeting ⭐

export const EpisodeContext = createContext({
// the default episode.
episode: DEFAULT_EPISODE,
setEpisode: (episode: string) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: should we standardize how we refer to the current episode? I have been calling it episodeId (referring to name_short like bc23) but I would be OK with using episode instead.

Copy link
Member Author

@acrantel acrantel Jun 25, 2023

Choose a reason for hiding this comment

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

We should def standardize this

What if we just call name_short episodeId and call name_long episodeName?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with this ^^

import { EpisodeContext } from "../../../contexts/EpisodeContext";
import { MemoryRouter } from "react-router-dom";

test('should link to default episode', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

My tests are currently not working but when they are we will have dozens... thinking this may be a good opportunity to use the API/UI: <test message> (STABLE/UNSTABLE) format for test messages?

Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
test('should link to default episode', () => {
test('UI: should link to default episode', () => {

Copy link
Member Author

Choose a reason for hiding this comment

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

update after discussion: we won't include (stable/unstable), but we will include api/ui

@lowtorola lowtorola self-requested a review June 25, 2023 17:39
@acrantel
Copy link
Member Author

addressed code review comments, let me know if this is good to merge!

@lowtorola
Copy link
Contributor

addressed code review comments, let me know if this is good to merge!

Looks good to me!!

Copy link
Contributor

@n8kim1 n8kim1 left a comment

Choose a reason for hiding this comment

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

lowell said it looked good so

@acrantel acrantel merged commit a571eae into frontend2 Jun 25, 2023
2 checks passed
@acrantel acrantel deleted the serena/routing branch June 25, 2023 20:11
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.

3 participants