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.

Sign up for GitHub

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

Jump to bottom

Banner 806 #886

Merged
merged 14 commits into from
Oct 24, 2023
Merged

Banner 806 #886

merged 14 commits into from
Oct 24, 2023

Conversation

popkinj
Copy link
Contributor

@popkinj popkinj commented Oct 20, 2023

Custom banner that can be configured for each environment. It is persistent ie. the user can not dismiss.

Not logged in

Info
Screenshot from 2023-10-18 11-16-16

Alert
Screenshot from 2023-10-18 11-18-47

Error or Serious
Screenshot from 2023-10-18 11-17-32

Turned off
Screenshot from 2023-10-18 11-19-33

All the same styles apply to when logged in... But located in a different spot. Not totally sure about this yet.... Open to suggestions.
Screenshot from 2023-10-20 12-04-41

@popkinj popkinj requested review from esune and loneil October 20, 2023 19:15
@loneil
Copy link
Contributor

loneil commented Oct 20, 2023

Don't really like the look of it in the header there in the last screenshot personally. Was thinking more like a topline banner above all the usual UX
image

@popkinj popkinj temporarily deployed to development October 20, 2023 19:27 — with GitHub Actions Inactive
@github-actions
Copy link

Deployment Resource Location
Traction
Tenant UI https://pr-886-traction-tenant-ui-dev.apps.silver.devops.gov.bc.ca
Innkeeper UI https://pr-886-traction-tenant-ui-dev.apps.silver.devops.gov.bc.ca/innkeeper
Aca-Py
Admin Swagger https://pr-886-traction-acapy-admin-dev.apps.silver.devops.gov.bc.ca/api/doc
Tenant Proxy Swagger https://pr-886-traction-tenant-proxy-dev.apps.silver.devops.gov.bc.ca/api/doc

Deployment URLs ready for review.

@popkinj popkinj temporarily deployed to development October 20, 2023 20:23 — with GitHub Actions Inactive
@popkinj popkinj temporarily deployed to development October 20, 2023 20:34 — with GitHub Actions Inactive
@popkinj popkinj temporarily deployed to development October 20, 2023 21:00 — with GitHub Actions Inactive
loneil
loneil requested changes Oct 20, 2023
View reviewed changes
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.

Seeing the banner with an empty message when the fields are turned off

image

@popkinj
Copy link
Contributor Author

popkinj commented Oct 20, 2023 •
edited by loneil
Loading

Seeing the banner with an empty message when the fields are turned off

image

Nice grab. By turned off do you mean the showMessage boolean?

@loneil
Copy link
Contributor

loneil commented Oct 20, 2023

Seeing the banner with an empty message when the fields are turned off

Nice grab. By turned off do you mean the showMessage boolean?

Not sure, whatever is set for values here in this PR has blank for text and false for showMessage and it's still showing.

image

@popkinj
Copy link
Contributor Author

popkinj commented Oct 20, 2023

Seeing the banner with an empty message when the fields are turned off

Nice grab. By turned off do you mean the showMessage boolean?

Not sure, whatever is set for values here in this PR has blank for text and false for showMessage and it's still showing.

image

ok... Thanks for the screenshot. That's definitely not the intended behaviour. I'll have a look.

@esune
Copy link
Member

esune commented Oct 23, 2023

Don't really like the look of it in the header there in the last screenshot personally. Was thinking more like a topline banner above all the usual UX

I agree with Lucas: the banner looks good on the sign-in page as an alert, but it isn't ideal when inside the Tenant UI.
@popkinj can we adjust it to look more similar to the screenshot/recommendation from Lucas? If it has to be that way on the m ain sign-in screen for consistency it's fine

@popkinj
Copy link
Contributor Author

popkinj commented Oct 23, 2023

Don't really like the look of it in the header there in the last screenshot personally. Was thinking more like a topline banner above all the usual UX

I agree with Lucas: the banner looks good on the sign-in page as an alert, but it isn't ideal when inside the Tenant UI. @popkinj can we adjust it to look more similar to the screenshot/recommendation from Lucas? If it has to be that way on the m ain sign-in screen for consistency it's fine

Sure. This may just be a placement thing with a couple css tweaks. Will play around a bit.

@popkinj
Copy link
Contributor Author

popkinj commented Oct 23, 2023

Screenshot from 2023-10-23 15-16-28

@popkinj
Copy link
Contributor Author

popkinj commented Oct 23, 2023

Screenshot from 2023-10-23 15-19-14

@popkinj popkinj temporarily deployed to development October 23, 2023 22:37 — with GitHub Actions Inactive
popkinj and others added 12 commits October 23, 2023 17:06
@popkinj @loneil
First crack at banner schema
7c91eb5 
Signed-off-by: Jamie Popkin <popkinj@littleearth.ca>
@popkinj @loneil
Second crack
519a6c7 
Signed-off-by: Jamie Popkin <popkinj@littleearth.ca>
@popkinj @loneil
This should do it for the back end
30b9110 
Signed-off-by: Jamie Popkin <popkinj@littleearth.ca>
@popkinj @loneil
Finally found where this was located
6decc12 
Signed-off-by: Jamie Popkin <popkinj@littleearth.ca>
@popkinj @loneil
Here is the front end of the notification
750c4d4 
For the login page at least

Signed-off-by: Jamie Popkin <popkinj@littleearth.ca>
@popkinj @loneil
Maybe here
70e7a76 
Signed-off-by: Jamie Popkin <popkinj@littleearth.ca>
@popkinj @loneil
Don't need this
cef3072 
Signed-off-by: Jamie Popkin <popkinj@littleearth.ca>
@popkinj @loneil
Updating the defaults
b8c4da5 
Signed-off-by: Jamie Popkin <popkinj@littleearth.ca>
@popkinj @loneil
Linting
4f77aa1 
Signed-off-by: Jamie Popkin <popkinj@littleearth.ca>
@popkinj @loneil
Fixing test sorta
80fd27c 
Signed-off-by: Jamie Popkin <popkinj@littleearth.ca>
@popkinj @loneil
Full width alert message
eb99b70 
Signed-off-by: Jamie Popkin <popkinj@littleearth.ca>
@loneil
Top banner fixes
edcfb56 
Signed-off-by: Lucas ONeil <lucasoneil@gmail.com>
@loneil loneil temporarily deployed to development October 24, 2023 17:04 — with GitHub Actions Inactive
@popkinj popkinj temporarily deployed to development October 24, 2023 17:17 — with GitHub Actions Inactive
@popkinj popkinj requested a review from loneil October 24, 2023 17:26
@popkinj
Copy link
Contributor Author

popkinj commented Oct 24, 2023

Tested a bunch of configurations and things are looking really good now.

loneil
loneil requested changes Oct 24, 2023
View reviewed changes
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.

Think we should remove unused config

charts/traction/templates/ui/configmap.yaml Outdated Show resolved Hide resolved
services/tenant-ui/config/custom-environment-variables.json Outdated Show resolved Hide resolved
@popkinj
Not using these yet
f47f0f3 
Signed-off-by: Jamie Popkin <popkinj@littleearth.ca>
@popkinj popkinj temporarily deployed to development October 24, 2023 21:53 — with GitHub Actions Inactive
@loneil loneil merged commit 2aa2e77 into main Oct 24, 2023
11 checks passed
@loneil loneil temporarily deployed to development October 24, 2023 22:01 — with GitHub Actions Inactive
@popkinj popkinj deleted the banner-806 branch October 24, 2023 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@loneil loneil loneil approved these changes

@esune esune Awaiting requested review from esune

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@popkinj @loneil @esune