Closed wbentley15 closed 8 months ago
Thanks for the PR @wbentley15
Yes, as you said, there are quite a few changes in here for a single PR. If I may suggest it, you should consider breaking these into atomic commits/PRs. There's a really good resource that goes into some detail about why atomic commits are preferred here: https://gist.github.com/webknjaz/a7362787a80067af8621a85a71746ca1
TL;DR - atomic commits/PRs are much easier to review and it isolates changes in a way that leads to a much clearer git history. There are many other benefits outlined in that link.
@oraNod I will be honest with you to say I do not have time to research the approach you provided. Need to move on to my other responsibilities. As mentioned, everything thing here is a requirement for launch. Check in with @Spredzy on Monday as I have already spoken with him. Have a great weekend!
FYI @oraNod I am aware the page named 'automation-adoption' is missing. I am still drafting that article. Goal is to have it into a PR next week. Saw it was the failure of the nox build for this PR.
@oraNod I will be honest with you to say I do not have time to research the approach you provided. Need to move on to my other responsibilities. As mentioned, everything thing here is a requirement for launch. Check in with @Spredzy on Monday as I have already spoken with him. Have a great weekend!
will do @wbentley15 and don't worry. I'm here to help out. enjoy your weekend.
Thank you @oraNod and enjoy your weekend too!
@wbentley15 Along with the CI fixes I think we need some changes for accessibility. Here is a link to the report for the current preview build: https://wave.webaim.org/report#/https://ansible-community-website--418.org.readthedocs.build/
It looks like there are some alerts for suspicious links, contrast errors, incorrect heading levels, and the embedded video:
@wbentley15 Another thing to note is that the embedded video causes overflow issues on smaller displays when you go into responsive design mode.
@wbentley15 It looks like I can't push directly to your branch so I opened a PR that you can merge to resolve some of the accessibility issues and CI checks: https://github.com/wbentley15/community-website/pull/1
@wbentley15 I think the color contrast for the catalyst section needs to be revisited. It's pretty tough to find a decent background color with that red foreground color. I've tried a few options here: https://webaim.org/resources/contrastchecker/
Would it be acceptable to use another one of our theme colors like cyan? That is fine on a white background.
@oraNod Yes, we can absolutely pivot from red to cyan for the heading color. Hope that makes adjusting the color contrasts easier.
@wbentley15 Hey, one more PR for you to merge into your branch: https://github.com/wbentley15/community-website/pull/2
I've set the font color to cyan for the catalyst section, which resolves the accessibility issue with color contrast.
I also added a title to the embedded video for accessibility. We still get an alert about the video but I've verified that captions are available so I think we're good in terms of actually being accessible. You can find the report for that pull request here:
https://wave.webaim.org/report#/https://oranod-website.readthedocs.io/en/latest/
Finally I modified the welcome video band so it is more responsive. When the display resizes to 992px the text and video stack on top of each other. When the display hits 430px the video resizes to 300px to avoid overflow and when the display hits 300px we just hide the welcome band.
I don't think anyone is going to be viewing the site on a device with a 300px width or lower display but I thought it seemed reasonable to just hide the video at that stage because it avoids issues and the site text is still usable. We can change it again if needed though.
@wbentley15 One more for you. Hopefully you'll agree with the slight changes to the wording on some of those catalyst fields. Just let me know if you have any nits and we can work through them.
After that the only other thing I see here that might need tweaking is the fact the alternate band colouring got messed up when the welcome band dropped. Now the tab menu and the Ansible band both have the same background colour:
It also looks odd to have the platform band on top of the bullhorn band. That means there are two bands on top of each other without the "Back to top" link, which sort of breaks the navigation a little.
Another thing with the bullhorn band at the bottom is that it creates a narrow band of white between just before the footer on mobile:
We can play around with this some but what if we either restore the bullhorn band to the previous order - or move the platform band to where the bullhorn was previously?
Sorry forgot to put the link to the pull request for your branch: https://github.com/wbentley15/community-website/pull/3
Howdy,
This PR consists of numerous suggestions per my leadership and adjustments committed to RH Marketing required for launch. Yes, there are a lot of changes so I apologize in advance for that. Wanted to make sure we would be cleared for launch. Certainly open to discussion on any adjustments. Just note any deviations will cause further delays in our launch. Goal is to get it out the door.
Thanks, Walter