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

Update website #130

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from
Draft

Update website #130

wants to merge 12 commits into from

Conversation

qu1queee
Copy link
Contributor

Changes

This is a complete rewrite of the website, mainly the landing page and the footer.

Comes with the necessary changes for the CNCF on-boarding as well.

Pardon my lack of experience in this area.

Submitter Checklist

  • Includes tests if functionality changed/was added
  • Includes docs if changes are user-facing
  • Set a kind label on this PR
  • Release notes block has been filled in, or marked NONE

Release Notes

NONE

Copy link
Contributor

openshift-ci bot commented Sep 17, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from qu1queee. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@pull-request-size pull-request-size bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 17, 2024
@qu1queee qu1queee added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 17, 2024
@pull-request-size pull-request-size bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 17, 2024
@qu1queee qu1queee force-pushed the qu1queee/update_website branch 3 times, most recently from 4804357 to ff9773a Compare September 17, 2024 14:34
Copy link
Member

@adambkaplan adambkaplan left a comment

Choose a reason for hiding this comment

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

First reaction is I love the new look! So much cleaner and gets rid of that poor-res container ship.

I want to propose merging this into a feature branch for a few reasons:

  • The maintainers are working with the Linux Foundation to issue a public statement/press release on the move. Timing the website update/refresh with this announcement makes sense to me.
  • I noticed a few important accessibility issues related to color contrast. Some I could trace back to the .scss files easily. The one I couldn't quite find is the .scss used to render command line monospace. That color blends in with the background, whereas the color text for yaml seems to be fine.
  • There are a few formatting bits I found - as you noted, there are areas of improvement since you aren't an expert on HTML/CSS.

I'd prefer we merge what we have somewhere and invite the rest of the community to contribute. The Docsy/hugo upgrades alone pay off a huge amount of tech debt.

The only thing that I think we have to address before merge is the color contrast issues. I found a few on the main page, and

@@ -17,15 +17,17 @@ clean: ## clean the build assets
install: ## install dependencies
bundle
npm install
hugo mod get
hugo mod graph
hugo mod get github.com/google/docsy
Copy link
Member

Choose a reason for hiding this comment

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

Nit - can we pin the version of Docsy with hugo mod get?

Comment on lines -22 to -23
netlify: submodule-init ## build the site for Netlify
git submodule update --init --recursive --depth 1
Copy link
Member

Choose a reason for hiding this comment

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

yay for no more submodules!

Copy link
Member

Choose a reason for hiding this comment

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

Note to myself (and for others benefit) - config.toml is now represented in hugo.yaml

Comment on lines +40 to +41
<h2>Shipwright supports popular tools such as ko, Cloud Native Buildpacks, and more!
<br> to build your container images.</h2>
Copy link
Member

Choose a reason for hiding this comment

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

For follow-up, line breaks and layout should be managed with CSS.

@@ -4,6 +4,7 @@ GEM
asciidoctor (2.0.17)

PLATFORMS
universal-darwin-23
Copy link
Member

Choose a reason for hiding this comment

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

Ack that this allows developers to test on Apple devices.

gap: 0.25rem;
margin-bottom: 1rem;
display: grid;
color: #28384b;
Copy link
Member

Choose a reason for hiding this comment

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

UX/accessibility issue - this color does not have strong enough contrast with the .lead-block-container background color.

text-align: left;
line-height: 100%;
letter-spacing: 0.62px;
color: #28384b;
Copy link
Member

Choose a reason for hiding this comment

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

UX/accessibility issue - not enough contrast with background

line-height: 1.6;
margin-bottom: 1.5rem;
margin-left: 0.41rem;
color: #7e93a7;
Copy link
Member

Choose a reason for hiding this comment

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

UX/accessibility issue - not enough contrast with background

@qu1queee qu1queee marked this pull request as draft September 19, 2024 06:20
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 19, 2024
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 27, 2024
@openshift-merge-robot
Copy link
Contributor

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants