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

Changed Filter Component Structure #165

Open
wants to merge 1 commit into
base: NEW-UI
Choose a base branch
from

Conversation

himanshujasuja1040
Copy link
Contributor

@himanshujasuja1040 himanshujasuja1040 commented Jun 14, 2021

🚨 Please review the guidelines for contributing to this repository.

Please check if the PR fulfills these requirements

  • Make sure you are requesting to NEW-UI. Don't request other protected Branches like staging/master
  • Make sure no conflicts are present in the code, if so please resolve it(Tip: Always fetch upstream)
  • Your Commit messages should make sense.
  • Don't push your package.lock.json as this project uses yarn.lock already.
  • Check your code additions will fail neither code linting checks nor unit test.

Describe your changes

  • What kind of change does this PR introduce? (Filter Component Structure Changed)

  • What is the current behavior? (Multiple Filter Option Select at one time)

  • What is the new behavior (if this is a feature change)?(User Can Only Select One Filter Type only)

  • Does this PR introduce a breaking change? (i.e changes that may require users to update/refactor existing istances of the application)

  • Other Information:

❤️ Thank you!

@vercel
Copy link

vercel bot commented Jun 14, 2021

Someone is attempting to deploy a commit to a Personal Account owned by @stephin007 on Vercel.

@stephin007 first needs to authorize it.

@himanshujasuja1040
Copy link
Contributor Author

@Justinnn07 @stephin007

@vercel
Copy link

vercel bot commented Jun 14, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/stephin007/cowin-vaccine-availablity-checker/9ihxw7HA2f9f996pzgT61may4EVQ
✅ Preview: https://cowin-vaccine-availablity-checker-git-fork-hi-502375-stephin007.vercel.app

@stephin007
Copy link
Owner

stephin007 commented Jun 14, 2021

@himanshujasuja1040 This has some issues,

  • If the filter values does not find any match it does not show the no results found component, for example if i select Chandigarh as state and Chandigarh as district, i filter for vaccine type of covaxin, it should ideally empty the vaccine list as there is no vaccine type of covaxin everything is covishield.
  • i select a state and district and later i switched to some other state and district , this does not update the vaccine list ,it shows the results of the former state which was selected.
  • If i select the filter type as vaccine type, it removes the whole vaccine list , it only shows data when i select any of the vaccine type i.e. vaccine array is only shown if i select either covaxin or covisheild same is the situation for other filter types.

Will add more if i find more issues.

@stephin007
Copy link
Owner

stephin007 commented Jun 14, 2021

@Justinnn07 can you do the code review to find if there is any issue with it or if there are any redundant parts.

@Justinnn07
Copy link
Collaborator

@Justinnn07 can you do the code review to find if there is any issue with it or if there are any redundant parts.

Sure!

Copy link
Collaborator

@Justinnn07 Justinnn07 left a comment

Choose a reason for hiding this comment

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

@himanshujasuja1040 Efforts appreciated!

Few points to be noted :-
image

  1. When there is no data of the current filter .. please show the null state component
  2. please align the btns of the filter

Check the comments, which @stephin007 mentioned earlier ..

Rest changes looks good to me!

Also, Please fill the PR template too! 😂

Thanks

@Justinnn07 Justinnn07 self-requested a review June 16, 2021 02:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants