Skip to content

Enhance 404 page #68

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

Merged
merged 4 commits into from
Nov 16, 2023
Merged

Enhance 404 page #68

merged 4 commits into from
Nov 16, 2023

Conversation

xscottxbrownx
Copy link
Contributor

This PR:

Resolves #59


Screenshots:

If API call is successful & doesn't return a video (which is an option):
Screenshot 2023-11-12 at 12 03 38 PM

If API call is unsuccessful or returns a video:
Screenshot 2023-11-12 at 11 52 37 AM

There is still the footer below the full screen photo.


Future Steps/PRs Needed to Finish This Work (optional):

Any styling or code quality discussions

@andycwilliams
Copy link
Member

🚀

(I'll review when I get home)

Copy link
Member

@andycwilliams andycwilliams left a comment

Choose a reason for hiding this comment

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

I'm liking this a lot better. Just have a few suggestions to try out.

Copy link
Contributor

@Jared-Krajewski Jared-Krajewski left a comment

Choose a reason for hiding this comment

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

See comment on state for loading image from api. Also might need to change text/button for contrast. We might end up relying on that darker green for white text on buttons.

Copy link
Contributor

@Jared-Krajewski Jared-Krajewski left a comment

Choose a reason for hiding this comment

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

approving to merge at your discretion, but still think something needs to be done with color/contrast of all elements in the blur box including the white in light green button (that color combo scores low on accessibility and needs to change throughout the page. That whole area is hard for me to read depending on the background image but I think its a contrast issue more than scaling. the two colors of green text also kinda bugs my eyes out. I tried putting the blur back to where you had it (10+ range) and using white text and that seemed to help.

@xscottxbrownx
Copy link
Contributor Author

approving to merge at your discretion, but still think something needs to be done with color/contrast of all elements in the blur box including the white in light green button (that color combo scores low on accessibility and needs to change throughout the page. That whole area is hard for me to read depending on the background image but I think its a contrast issue more than scaling. the two colors of green text also kinda bugs my eyes out. I tried putting the blur back to where you had it (10+ range) and using white text and that seemed to help.

I'm going to leave this for the full accessibility review issue #55

The button is just an MUI button with the primary color. We can figure out the right color combo and switch them all at once.

I updated text color to this, which to me looks okay for contrast, but I have not tested with accessibility tools. (the text, not the button which I agree isn't good.)
Screenshot 2023-11-15 at 9 40 09 PM

@xscottxbrownx xscottxbrownx merged commit 20f7710 into main Nov 16, 2023
@xscottxbrownx xscottxbrownx deleted the issue-59/update-404-page branch November 16, 2023 05:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

404 Page Enhacement
3 participants