Closed amalnanavati closed 1 year ago
@amalnanavati Same page renders differently in different browsers. ipad mini rendering plate locator screen in Safari:
in Google Chrome:
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?
Amal's comment about handling magic numbers linked here.
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.-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
:
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.scaleWidthHeightToWindow
to take the margins as pixels.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).See these images of how the LiveVideoModal renders now, and take a look at the commit here:
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.
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.
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.
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: