-
Notifications
You must be signed in to change notification settings - Fork 0
Cais gateway UI changes #12
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
base: base-sha/d494bc8c6ad4c6f1008838b2a8d91e044ab53d63
Are you sure you want to change the base?
Cais gateway UI changes #12
Conversation
This pull request was cloned from Note: the URL is not a link to avoid triggering a notification on the original pull request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @brendanator - I've reviewed your changes and they look great!
General suggestions:
- Ye be makin' the seas of UI much more navigable, but don't forget to keep a weather eye on the horizon for cross-browser compatibility, matey.
- Arrr, while ye have set the sails for a grander UI, ensure ye not be overlookin' the responsiveness for those pilferin' the seas on mobile vessels.
- Ye've done well to swab the deck of bugs, but keep yer cannons ready for any that might resurface with future tides.
- Chart a course for accessibility, me hearty, so all sailors, regardless of their abilities, can navigate these waters.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 4 issues found
- 🟢 Complexity: all looks good
- 🟢 Docstrings: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
html { | ||
height: 100%; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code_refinement): Setting the HTML height to 100% is a good practice for full-height app layouts, ensuring that the gradient background covers the entire viewport regardless of the content size.
html { | |
height: 100%; | |
html { | |
height: 100%; | |
min-height: 100%; /* Ensures that the gradient background covers the entire viewport even if the content is smaller */ | |
} |
@@ -1,14 +1,184 @@ | |||
html { | |||
height: 100%; | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code_refinement): The use of
rgba(0,0,0,.5)
for the .begin-box
background provides a nice translucent effect, enhancing the visual appeal of the modal. However, consider the readability of text over this background, especially in various lighting conditions.
} | |
.begin-box { | |
background-color: rgba(0,0,0,0.5); /* Consider adjusting the opacity or adding a text shadow for better readability */ | |
color: #fff; /* Ensure high contrast for readability */ | |
text-shadow: 1px 1px 2px black; /* This can help text stand out more */ | |
} |
Revamped the UI and fixed several UI bugs. Only tested on desktop.