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

Make the App Fully Responsive #9

Closed
amalnanavati opened this issue Apr 14, 2023 · 5 comments · Fixed by #60
Closed

Make the App Fully Responsive #9

amalnanavati opened this issue Apr 14, 2023 · 5 comments · Fixed by #60
Assignees
Labels
critical-path Issues that are critical path for deployment good first issue Good for newcomers

Comments

@amalnanavati
Copy link
Contributor

Currently, there are places in the app where we hardcore margins to center buttons/components on the screen. For example, see this, this, and this.

This issue encompasses the following:

  1. Thoroughly test every page/component of the app for a wide variety of screen sizes (desktop, tablet, multiple smartphone sizes, multiple arbitrary sizes) and ensure that the screen renders as desired (i.e., in both an aesthetic and functional manner) for all screen sizes.
  2. Where rendering messes up for different screen sizes, implement sustainable fixes so that it will correctly render for all screen sizes.
@amalnanavati amalnanavati added the good first issue Good for newcomers label Apr 17, 2023
@Raidakarim Raidakarim self-assigned this Apr 17, 2023
@Raidakarim Raidakarim mentioned this issue Apr 17, 2023
2 tasks
@Raidakarim
Copy link
Contributor

@amalnanavati Same page renders differently in different browsers. ipad mini rendering plate locator screen in Safari:
Screen Shot 2023-04-18 at 1 31 44 PM
in Google Chrome:
Screen Shot 2023-04-18 at 1 33 17 PM
Notably, there's seems to be a video space in chrome that is not shown in safari.
Shouldn't we target specific browser, device (e.g., iphone 7 pro, samsung galaxi S8+) to fix the app UI designs?

@Raidakarim
Copy link
Contributor

Amal's comment about handling magic numbers linked here.

@amalnanavati
Copy link
Contributor Author

Since both @Raidakarim and @atharva-kashyap had questions about the -70 in helpers.jsx, I decided to address that as part of #5, to provide an example of how we should make the code as responsive as possible.

There were a few issues:

  • -70 was a margin specific to the live video modal, but was getting used in a generic helper function that was also being used for PlateLocator.jsx
  • -70 was only being applied to the bottom and not the top. Why? Even if a hardcoded value is necessary, it must be accompanied by a comment in the code explaining why.
  • The bottom margin of -70 would not change as device size changes, and it was unclear whether that was desired.

To address this, I did the following, first focusing on LiveVideoModal.jsx and then PlateLocator.jsx:

  1. First, I changed scaleWidthHeightToWindow to take in a user-specified margin which was a proportion of the window. I thought that would address the responsiveness issue, but then when I tried it on different browser sizes the margin size changed, which looked weird.
  2. I then considered why the margin size looked weird. It's because there was a set top margin in the Modal CSS, and my visual expectation was that that margin would be mirrored on all sides.
  3. Therefore, in this case we do want a set margin, as opposed to one that changes with the window. But it still shouldn't be hardcoded into this function, but instead passed as a parameter so different Components that use this function can customize the margin as necessary. So I changed scaleWidthHeightToWindow to take the margins as pixels.
  4. There was another issue with scaleWidthHeightToWindow, which is that it used the overall window dimensions, as opposed to the dimensions available to the image. For example, in LiveVideoModal.jsx there is a title and header, so using the window height is greater than the actual height we want. Having the margin as pixels allowed us to account for factors like that (e.g., passing in a greater marginTop due to that header).
  5. I then passed in the exact margin values that correspond to the space and/or desired space from the edges of the window. Although these are magic numbers, I have a comment explaining where they come from.
  6. Since the CSS files for Modal stored margins as "rem" units, I had to write another helper function to convert REM to px.

See these images of how the LiveVideoModal renders now, and take a look at the commit here:

Screenshot 2023-04-18 at 9 20 29 PM

Screenshot 2023-04-18 at 9 20 55 PM

Screenshot 2023-04-18 at 9 21 15 PM

Note that I'm not convinced that this is the best solution, since using the entire window seems a bit suspect when only a small portion of the window is actually available for the image. Perhaps we should instead compute the window width/height in the component and pass that in as a parameter. But for now, this is a functional solution.

@amalnanavati
Copy link
Contributor Author

BTW @Raidakarim, re. responsiveness of the PlateLocator, I think you should make it so that the entire UI (image, arrow buttons, and Done) fills the whole screen, as opposed to the image expanding to fill the whole width. In other words, in the second image below, the buttons should still appear on the same screen IMO, as opposed to requiring scrolling.

Screenshot 2023-04-18 at 9 30 30 PM
Screenshot 2023-04-18 at 9 30 51 PM

@amalnanavati amalnanavati added the critical-path Issues that are critical path for deployment label May 9, 2023
@Raidakarim
Copy link
Contributor

Here is a spreadsheet and a google doc with some initial thoughts of mine. The comments on this spreadsheet include feedback for improvement from Amal @amalnanavati.

I currently don't have the time to actively work on this issue. So, if anyone in the web app team wants to assign it to themselves, please feel free to. Otherwise, I can self-assign this issue to myself in the future when I am done with my current issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
critical-path Issues that are critical path for deployment good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants