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

[#11150] Search page: Remove unnecessary panel #11679

Conversation

tamrakarsweta220
Copy link
Contributor

@tamrakarsweta220 tamrakarsweta220 commented Mar 28, 2022

Fixes #11150

PR Checklist

Ensure that you have:

  • Read and understood our PR guideline: https://github.com/TEAMMATES/teammates/blob/master/docs/process.md#step-4-submit-a-pr
    • Added the issue number to the "Fixes" keyword above
    • Titled the PR as specified in the above mentioned document
  • Made your changes on a branch other than master and release
  • Gone through all the changes in this PR and ensured that:
    • They addressed one (and only one) issue
    • No unintended changes were made
  • Run and passed static analysis: ./gradlew lint and npm run lint
  • Added/updated tests, if changes in functionality were involved
  • Added/updated documentation to public APIs (classes, methods, variables), if applicable

Outline of Solution

I made two changes to the instructions on the student search page.

  1. Updated the search instruction to let users know that they can search by name or email
  2. Gave an example for using quotations in the search bar to search for specific phrases at the end of the instruction.

I also removed the "student" header and the outer panel from the search result table.

This is how the changes currently look like:
Screen Shot 2022-03-27 at 8 59 59 PM

@teammates-bot

This comment was marked as resolved.

1 similar comment
@teammates-bot

This comment was marked as resolved.

@teammates-bot

This comment was marked as resolved.

Copy link
Contributor

@FergusMok FergusMok left a comment

Choose a reason for hiding this comment

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

Hello, thank you for contributing to TEAMMATES. It seems like some linting tests are failing too

You can refer to the documentation for linting . To update the snapshots, you can run npm run test -- --updateSnapshot, or alternatively, run npm run test, and then press the button "u".

@fsgmhoward
Copy link
Member

fsgmhoward commented Mar 28, 2022

From your screenshot, it looks like all your tests are failing. Have you executed this before you run the test:

./gradlew generateTypes

If that is the case, you will need to execute this before you run the test. This command generates frontend types from backend description files. After that, you can run test and update snapshot based on displayed information (see @FergusMok 's comment above).

npm run test

Even though the change is minor, it is still suggested to follow steps in Development Guide to run the development server and test the functionality.

@teammates-bot
Copy link

Hi @tamrakarsweta220, these parts of your pull request do not appear to follow our contributing guidelines:

  1. PR Description
    • Should contain GitHub keyword to auto-close issue it fixes: Refer here for a list of accepted keywords.

@tamrakarsweta220
Copy link
Contributor Author

I followed the instructions as suggested above by @fsgmhoward and @FergusMok and updated the snapshot, so the component tests have passed. I am still having issues with lint though. I tried running the npm run lint command for the master branch too, but it's the same. The lint checks fail in the master branch too.

Also, the teammates bot keeps telling me that my PR "Should contain GitHub keyword to auto-close issue it fixes: Refer here for a list of accepted keywords." I wrote "fixes" based on the documentation but it's still showing me this error. I capitalized the "f" in "fixes" and then put it in lowercase too, but the error didn't go away. So, I am not sure what is causing the error. Is there something else that I am missing?

@teammates-bot

This comment was marked as resolved.

@fsgmhoward
Copy link
Member

I followed the instructions as suggested above by @fsgmhoward and @FergusMok and updated the snapshot, so the component tests have passed. I am still having issues with lint though. I tried running the npm run lint command for the master branch too, but it's the same. The lint checks fail in the master branch too.

Also, the teammates bot keeps telling me that my PR "Should contain GitHub keyword to auto-close issue it fixes: Refer here for a list of accepted keywords." I wrote "fixes" based on the documentation but it's still showing me this error. I capitalized the "f" in "fixes" and then put it in lowercase too, but the error didn't go away. So, I am not sure what is causing the error. Is there something else that I am missing?

Usually we do with "Fixes". I am not sure whether it is accepted or not in lower case.

For the lint issue, I see that all checks passed already. It fails on your local machine because the lint program takes auto-generated files, which will fail it. You have to take a look at which file fails the linting test. If it is not those you made changes, then you are fine. If you are interested, the related issue is this: #11564

@teammates-bot
Copy link

Hi @tamrakarsweta220, these parts of your pull request do not appear to follow our contributing guidelines:

  1. PR Description
    • Should contain GitHub keyword to auto-close issue it fixes: Refer here for a list of accepted keywords.

@wkurniawan07 wkurniawan07 added the s.ToReview The PR is waiting for review(s) label Mar 31, 2022
@samuelfangjw samuelfangjw requested review from FergusMok and fsgmhoward and removed request for FergusMok April 4, 2022 17:34
Copy link
Contributor

@FergusMok FergusMok left a comment

Choose a reason for hiding this comment

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

Good job, the table looks good. Just some small considerations, and remember to update the branch to master

@tamrakarsweta220
Copy link
Contributor Author

@FergusMok Hi, I just made the changes you suggested. So, I'm commenting for code review again. Thanks!

Copy link
Member

@fsgmhoward fsgmhoward left a comment

Choose a reason for hiding this comment

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

LGTM.

@FergusMok FergusMok added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.ToReview The PR is waiting for review(s) labels Apr 8, 2022
Copy link
Contributor

@FergusMok FergusMok left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@wkurniawan07 wkurniawan07 added this to the V8.14.0 milestone Apr 8, 2022
@madanalogy madanalogy added the c.Task Other non-user-facing works, e.g. refactoring, adding tests label Apr 9, 2022
@madanalogy madanalogy added c.Feature User-facing feature; can be new feature or enhancement to existing feature c.Bug Bug/defect report and removed c.Task Other non-user-facing works, e.g. refactoring, adding tests c.Feature User-facing feature; can be new feature or enhancement to existing feature labels Apr 9, 2022
@madanalogy madanalogy merged commit 390ac30 into TEAMMATES:master Apr 9, 2022
@wkurniawan07 wkurniawan07 modified the milestones: V8.14.0, V8.13.1 Apr 10, 2022
FergusMok pushed a commit to FergusMok/teammates that referenced this pull request Apr 11, 2022
)

* rephrased instructions for searching students

* removed students title from the search component

* removed blue outline of the search results panel

* updated snapshot

* fixed some lint errors

* rephrased instructions to include more fields for search
ziqing26 pushed a commit to ziqing26/teammates that referenced this pull request Apr 11, 2022
)

* rephrased instructions for searching students

* removed students title from the search component

* removed blue outline of the search results panel

* updated snapshot

* fixed some lint errors

* rephrased instructions to include more fields for search
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c.Bug Bug/defect report s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Search page: Remove unnecessary panel
6 participants