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

[FIX] Issue #122 - Responsive UI #147

Open
wants to merge 17 commits into
base: NEW-UI
Choose a base branch
from
Open

Conversation

wise-introvert
Copy link
Contributor

@wise-introvert wise-introvert commented Jun 4, 2021

Note: These changes have been made only to the files mentioned ( indirectly ) in the issue. I haven't looked at the other files 😅.

Major Changes

  • Completely refactored the Home Page and VaccineDataSingle component to be more responsive and scalable
  • Extracted some of the redundant components to their own lexical environment ( Will be creating issues for code blocks that can be refactored the same way )
  • Created a single-source-of-truth for CSS ( for the Home page and the component ( mentioned above ) ) at /src/lib/useClasses.js

Minor Changes

  • Replaced some of the conditions with regular expressions ( Still a lot of places where this change can be made )

@vercel
Copy link

vercel bot commented Jun 4, 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/2PMBv7X4TkcqxdCDJ9c27yyghShQ
✅ Preview: https://cowin-vaccine-availablity-checker-git-fix-issue-122-stephin007.vercel.app

@wise-introvert wise-introvert changed the title Fix/issue 122 [FIX] Issue #122 - Responsive UI Jun 4, 2021
Copy link
Contributor Author

@wise-introvert wise-introvert left a comment

Choose a reason for hiding this comment

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

Fixed in 6957b5a

src/components/NullState.js Outdated Show resolved Hide resolved
@wise-introvert wise-introvert marked this pull request as ready for review June 5, 2021 03:57
@stephin007 stephin007 added the enhancement New feature or request label Jun 6, 2021
@stephin007
Copy link
Owner

hey @wise-introvert few points:

  • Point 1: When search criteria is search by district, when we change the state, the district does not change according to the changed state. But initially when we set the state it changes the district but when we change it, the district doesn't change.

  • Point 2: Say for example, user is in "search by district" , user faces the above situation and tries to change the search criteria to pincode and then goes back to search by district, the state still remains that means the user still faces the same issue as above.

  • Point 3: In vaccineDataSingle.js in the custom badge, you are sending a prop of background, which has a path "/covi/gi.test(vaccine?.vaccine)" i am not able to find any reference to this, it would be great if you could tell me what is this?

  • Point 4: " ...rest ", the use of this?(This is present in the index.js inside badge folder)

  • Point 5: In vaccineDataSingle.js , Opening and closing time is hidden at larger screens, any specific reason for that.

  • Point 6: In Home.js , in line 191 it says " if(true) ", what is the condition here? i mean if what is true, that the next piece of code will work.

  • Point 7: Just needed an general over view of the pages/home/index.js, Right now, is our home page rendering from this component?

@stephin007
Copy link
Owner

Also, please remove all the unused variables, as this wont let the deployment to happen.

image

@wise-introvert
Copy link
Contributor Author

wise-introvert commented Jun 13, 2021

@stephin007

  • Point 1: Fixed in 5e2fc56
  • Point 2: Not necessary now that issue described in Point 1 has been resolved.
  • Point 3: This is a Regular Express test. I'm basically checking if the value of vaccine?.vaccine matches the regular expression /covi/gi
  • Point 4: This is called a spread operator. When you want to pass dynamic properties to a component, you can use this feature to group all those properties in a variable ( rest in this case ) and then simply use it inside the component. Learn more
  • Point 5: There are two instances of "Opening" and "Closing" times. These instances are rendered interchangeably, based on the screen size. So, in code, it looks like they are hidden but in reality, they are just being rendered conditionally ( Let me know if you want further clarification 😃 )
  • Point 6: I forgot to remove that condition 😅. Will remove it after I publish this comment.
  • Point 7: Right now, pages/home/index.js is being rendered as /home-page in the browser. I did this to avoid any conflicts with the existing routes. Once the staging deployment of this PR is reviewed, I can switch to the actual route ( /vaccines, I think ).

@wise-introvert
Copy link
Contributor Author

Also, please remove all the unused variables, as this wont let the deployment to happen.

image

@stephin007 Is there any way you can disable the react-hooks warnings? Adding state methods to useEffect dependency array will result in an indefinite loop, crashing the application.

@Justinnn07
Copy link
Collaborator

Justinnn07 commented Jun 13, 2021

Also, please remove all the unused variables, as this wont let the deployment to happen.
image

@stephin007 Is there any way you can disable the react-hooks warnings? Adding state methods to useEffect dependency array will result in an indefinite loop, crashing the application.

@wise-introvert we can just paste // eslint-disable before the line where the warning is shown ..

and please resolve the conflicts!

@stephin007
Copy link
Owner

@wise-introvert can you add the latest changes done in the new-ui branch!

  • as right now we have removed the existing pagination.
  • There is a new page which is present in the navbar called as "Covid19 Information"

These are some changes which i saw on UI, i am reviewing the rest of changes!

@wise-introvert
Copy link
Contributor Author

Okay, I'll sync this, feature branch with the NEW-UI branch

@stephin007
Copy link
Owner

Hey @wise-introvert , Regarding point 4 : i know about spread operator :P , i was asking about the word rest here , was this declared somewhere or does it just mean to send all the rest of the props? Can we some different words like "other" as it was given in the article.

@wise-introvert
Copy link
Contributor Author

rest is like "i" and "j" are to for loop. It's what's usually used to denote "rest" of the props a component can receive. It can easily be changed though 😀

@stephin007
Copy link
Owner

No No @wise-introvert , dont change just wanted to understand that! :)

}-${year}`;
};

const refactor = (_vaccine) => {
Copy link
Owner

Choose a reason for hiding this comment

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

What is this refactor function doing? Please explain

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stephin007, there are two types of responses that the application receives from the vaccines endpoint: "Find by district/pincode" and "Find by district/pincdoe & date". Before the app renders either of these responses, they need to be converted into an object that follows only one, fixed structure ( as opposed to the two, legacy structures ). The refactor method performs this "conversion"

@stephin007
Copy link
Owner

hey @wise-introvert i hope you will be fixing the conflicts so that we merge to NEW-UI

@wise-introvert
Copy link
Contributor Author

Definitely. Sorry for the delay

@stephin007
Copy link
Owner

Awesome @wise-introvert , i have mentioned you in some comments it would be great if you could address it as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request merge-conflicts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants