Feature/mobile camera admin #312
Conversation
| color: 'var(--colors-gray7)', | ||
| ...(!isSearchable && { | ||
| '> div': { margin: '2px' }, | ||
| '> input': { display: 'none' }, |
There was a problem hiding this comment.
For some reason, when isSearchable is false, it still adds an input element to the dom which messes up the padding. This fixes the issue
There was a problem hiding this comment.
This makes me a bit nervous - have made sure this doesn't have any unintended consequences on desktop on other fields where isSearchable is false? Also for these kinds of notes can you add them as comments so future devs know why this is there?
Updating this: this does make all selects shorter across the app on desktop. This is an interesting catch, but I'm not sure how comfortable I am making this change in general without a thorough evaluation of the implications, and if we do roll with it it should be it's own PR, IMO. How necessary is this to make now? Is it important for the mobile UI?
| </> | ||
| } | ||
| > | ||
| {cam.deployments.map((dep) => { |
There was a problem hiding this comment.
Refactored this into DeploymentItem.jsx
|
Please feel free to start looking through the code. I will deploy to dev this week and give a final look over that nothing is obviously broken. Thank you! |
|
Few issues to fix after testing on dev:
|
|
Hi @nathanielrindlaub, this should be ready for review. I've poked it around on dev. Some things that I'd be grateful if you could look at are adding a deployment, deleting a camera, and deleting a deployment. I don't have the info to be able to re-add them so I didn't want to follow through deleting them but we should make sure that the middleware and redux updates don't break anything. |
| {image && ( | ||
| <> | ||
| <ImageMetadata image={image} /> | ||
| <LoupeDropdown image={image} /> |
There was a problem hiding this comment.
It seems like this got removed again in the most recent merge to main so readding it here
There was a problem hiding this comment.
I think this needs some more testing and a thorough side-by-side comparison with prod to make sure nothing unexpected changed. I didn't do a full review because I'm noticing a bunch of things that are broken, which I'll list here but I would not consider this comprehensive. The things I caught (mostly in Firefox for iOS) were:
Issues with the actions in the "..." menus:
- Creating new deployments doesn't seem to work. The form seemed to submit fine but then the new deployments don't show up (I only tried on mobile)
- Changing the camera's serial number causes the front end to crash
- When trying to delete a camera, the Keyboard covers the input so you can't type "permanently delete"
Unrelated to this PR (I just tested on prod) but I noticed that we don't confirm that a wireless camera registration was successful. I'll make a separate ticket for that. The Wireless cam registration and unregistration did work here though.
Styling issues:
- Deployment names are shortened to the point of being unreadable. If you are looking to save space, I would hide the deployment date ranges entirely (which seem to be broken anyhow):
2. not sure how controllable this is, but every time I select an input field the top like ~60 pixels of the modal disappears then reappears
3. The spinner overlay doesn't cover the bottom part of the modal after submitting "add deployment":
- There are styling issues this PR introduced on desktop. Here are two I noticed but there may be more:
| color: 'var(--colors-gray7)', | ||
| ...(!isSearchable && { | ||
| '> div': { margin: '2px' }, | ||
| '> input': { display: 'none' }, |
There was a problem hiding this comment.
This makes me a bit nervous - have made sure this doesn't have any unintended consequences on desktop on other fields where isSearchable is false? Also for these kinds of notes can you add them as comments so future devs know why this is there?
Updating this: this does make all selects shorter across the app on desktop. This is an interesting catch, but I'm not sure how comfortable I am making this change in general without a thorough evaluation of the implications, and if we do roll with it it should be it's own PR, IMO. How necessary is this to make now? Is it important for the mobile UI?
…ure/mobile_camera_admin
|
Working through this, I've addressed the following:
|
Context
Adjusts the styling of the camera admin mobile to be mobile friendly.