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

MK Implemented BasicCourseTableIndexPage and corresponding test #10

Merged
merged 3 commits into from Nov 28, 2022

Conversation

mariekarpinska
Copy link
Contributor

@mariekarpinska mariekarpinska commented Nov 18, 2022

Implemented BasicCourseSearchIndexPage.js so that it can be moved from the homepage into a separate page clickable from the menu bar. Additionally, implemented a test for the index page, BasicCourseSearchIndexPage.test.js.
These will be used to close issue 9: Swap the Section Search and the Description Search, which @masonma21 is closing by adapting AppNavBar and App.js and AppNavBar test to reflect the swap of basic course description search (from the homepage) and section search (from the menu bar).

Passed all frontend tests:
Screen Shot 2022-11-17 at 6 04 32 PM

Achieved Code Coverage:
Screen Shot 2022-11-17 at 6 05 14 PM

@mariekarpinska mariekarpinska added the f22-7pm-1 f22-7pm-1 label Nov 18, 2022
@andrewpengucsb andrewpengucsb added the FIXME: Peer CR Please get a peer code review label Nov 18, 2022
@pconrad pconrad force-pushed the MKandMM-SwapSectionCourseDescription branch 3 times, most recently from 5cd251c to aed3c1e Compare November 20, 2022 17:27
@Abelatnafu
Copy link
Contributor

Everything looks great to me. LGTM.

Copy link
Contributor

@pconrad pconrad left a comment

Choose a reason for hiding this comment

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

This PR adds a new page, but there is no way to access this new page yet, so it can't be tested in the app.

But what you could do (and should do so that this can be tested), is to add a story for it in the storybook.

I've done that for you this time.

That way once this job finishes:

image

At the -docs-qa site, you'll have a link to the storybook entry that you can include in the PR description like this:

image

And this link takes you to the new page:

https://ucsb-cs156-f22.github.io/f22-7pm-courses-docs-qa/storybook-qa/MKandMM-SwapSectionCourseDescription/?path=/story/pages-basiccoursesearch-basiccoursesearchindexpage--default

pconrad
pconrad previously approved these changes Nov 22, 2022
Copy link
Contributor

@pconrad pconrad left a comment

Choose a reason for hiding this comment

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

I added a storybook entry.. that's the only problem with the original PR. I took care of it for you this time to show how it's done; next time I'll ask the team to do it.

pconrad added a commit that referenced this pull request Nov 22, 2022
pc - update ci-cd to make sure jacoco and pitest coverage threshold i…
Abelatnafu
Abelatnafu previously approved these changes Nov 23, 2022
Copy link
Contributor

@Abelatnafu Abelatnafu left a comment

Choose a reason for hiding this comment

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

LGTM.

@andrewpengucsb andrewpengucsb added FIXME: Merge Conflicts Please address the merge conflicts in this PR and removed FIXME: Peer CR Please get a peer code review labels Nov 23, 2022
@pconrad pconrad added FIXME: CI/CD Failure One or more CI/CD checks is not green FIXME: Merge Conflicts Please address the merge conflicts in this PR FIXME: Peer CR Please get a peer code review and removed FIXME: Merge Conflicts Please address the merge conflicts in this PR labels Nov 23, 2022
@pconrad pconrad added 5 5 pts and removed FIXME: Peer CR Please get a peer code review FIXME: CI/CD Failure One or more CI/CD checks is not green FIXME: Merge Conflicts Please address the merge conflicts in this PR labels Nov 28, 2022
@pconrad pconrad merged commit f49e98b into main Nov 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 5 pts f22-7pm-1 f22-7pm-1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants