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

Add responsive sizing of VCAuthn login page for mobile devices #430

Merged
merged 16 commits into from
Mar 18, 2024

Conversation

Gavinok
Copy link
Contributor

@Gavinok Gavinok commented Mar 5, 2024

This PR resolves #420 using media queries as well as bootstrap.

Here is the new desktop view

2024-03-05_12-31-10

2024-03-05_12-28-05

To ensure uniform rendering of the boarder it no longer uses a dedicated svg and instead uses CSS boarders

On mobile devices the card style is removed and instead the entire screen is used to make the most of the smaller displays.

2024-03-05_12-34-14

2024-03-05_12-44-45

@Gavinok Gavinok requested review from loneil and esune March 5, 2024 20:47
@Gavinok Gavinok force-pushed the mobile-adaptive-qr-display branch from c589f8f to edcf0f1 Compare March 5, 2024 20:51
@Gavinok
Copy link
Contributor Author

Gavinok commented Mar 5, 2024

With the removal of media queries the css is simplified. I have also migrated the button css to make use of bootstrap. Here is the updated look on desktop

2024-03-05_15-50-38

and on mobile

2024-03-05_15-51-44

@esune
Copy link
Member

esune commented Mar 6, 2024

@Gavinok can you resolve the conflicts - likely with code from the last merged PR?

@Gavinok
Copy link
Contributor Author

Gavinok commented Mar 6, 2024

@esune working on this now

Gavinok added 4 commits March 6, 2024 10:59
Signed-off-by: Gavin Jaeger-Freeborn <[email protected]>
Signed-off-by: Gavin Jaeger-Freeborn <[email protected]>
Signed-off-by: Gavin Jaeger-Freeborn <[email protected]>
@Gavinok
Copy link
Contributor Author

Gavinok commented Mar 6, 2024

@esune resolved the conflict

esune
esune previously approved these changes Mar 6, 2024
Copy link
Member

@esune esune left a comment

Choose a reason for hiding this comment

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

LGTM based on screenshots and what demonstrated during live review.

Copy link
Contributor

@loneil loneil left a comment

Choose a reason for hiding this comment

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

Holding this as deep link is no longer delivering request message (working on main fine)

@loneil
Copy link
Contributor

loneil commented Mar 6, 2024

@Gavinok see comment above, not working with form action, needs to be an href link I believe.

Probably: https://stackoverflow.com/questions/45542773/how-to-style-an-anchor-as-a-button-in-bootstrap-4

@loneil
Copy link
Contributor

loneil commented Mar 6, 2024

Can we do some additional tweaks to the widths of the "header-desc" (and possibly other) containers?

They're thinner in which is crowding texts.

Existing:
image

Current:

Initial text (also could use a top margin here when opening the "other device" section)
image

Declined
image
(Probably same with approved and expired states)

Scanned mask over the qr code is thinner
image

@Gavinok
Copy link
Contributor Author

Gavinok commented Mar 6, 2024

@Gavinok see comment above, not working with form action, needs to be an href link I believe.

Probably: https://stackoverflow.com/questions/45542773/how-to-style-an-anchor-as-a-button-in-bootstrap-4

Dang that was what made is able to remove all that custom button CSS :(

@esune
Copy link
Member

esune commented Mar 6, 2024

I'm wondering if we should tackle the UI redesign/tweaks as part of the full move to Bootstrap, rather than potentially do the work twice? #431

@loneil
Copy link
Contributor

loneil commented Mar 6, 2024

@Gavinok see comment above, not working with form action, needs to be an href link I believe.
Probably: https://stackoverflow.com/questions/45542773/how-to-style-an-anchor-as-a-button-in-bootstrap-4

Dang that was what made is able to remove all that custom button CSS :(

No should just need to change button to a I just did that on my local and it worked.

@Gavinok
Copy link
Contributor Author

Gavinok commented Mar 6, 2024

Reference in new issue
Edit
Hide
Delete

It was more to do with the css not indicating the button was disabled but I was able to work around this anyways

@Gavinok Gavinok force-pushed the mobile-adaptive-qr-display branch from e0fdc1e to 6bf78a4 Compare March 6, 2024 22:16
Signed-off-by: Gavin Jaeger-Freeborn <[email protected]>
@Gavinok
Copy link
Contributor Author

Gavinok commented Mar 6, 2024

@loneil is it just me or does the qr code expire extremely fast now? Did I make a change that could effect that?

Signed-off-by: Gavin Jaeger-Freeborn <[email protected]>
@Gavinok
Copy link
Contributor Author

Gavinok commented Mar 6, 2024

Can we do some additional tweaks to the widths of the "header-desc" (and possibly other) containers?

They're thinner in which is crowding texts.

Existing: image

Current:

Initial text (also could use a top margin here when opening the "other device" section) image

Declined image (Probably same with approved and expired states)

Scanned mask over the qr code is thinner image

I have just updated this. There should be a little more space. I figured we should add some line breaks for more consistent text formatting while I was at it.

@Gavinok Gavinok requested a review from loneil March 14, 2024 18:34
Copy link
Contributor

@loneil loneil left a comment

Choose a reason for hiding this comment

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

Works for me, should be good to deploy for more device testing in dev. Can refine further with bootstrap framework.

@Gavinok Gavinok merged commit bc4c385 into main Mar 18, 2024
5 checks passed
@loneil loneil deleted the mobile-adaptive-qr-display branch May 3, 2024 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Responsive sizing for VCAuthn login page on mobile devices
3 participants