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

Rework the footer with link lists #606

Merged
merged 4 commits into from
Jan 11, 2022
Merged

Rework the footer with link lists #606

merged 4 commits into from
Jan 11, 2022

Conversation

palewire
Copy link
Collaborator

@palewire palewire commented Jan 9, 2022

This fixes #601.

Before:
Screenshot from 2022-01-09 07-23-37

After:
Screenshot from 2022-01-09 07-23-58

@palewire palewire added this to the Streamline the site milestone Jan 9, 2022
@palewire palewire requested a review from choldgraf January 9, 2022 15:29
@palewire palewire changed the title Reworked the footer with link lists Rework the footer with link lists Jan 9, 2022
Copy link
Collaborator

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

The code changes look good to me - for style, what do you think about centering this 3-column list of projects, so there's not so much whitespace to the right, and so it follows page content structure more closely?

@palewire
Copy link
Collaborator Author

palewire commented Jan 9, 2022

The trick there is that they will lose their shared left alignment with the logo and copyright text at the bottom.

I suspect the happy medium would be some kind of max width set on all content in the footer, which would prevent the flow from leaving a lot of space to the right on large device widths. However, considering the rest of the page design, there's not an obvious value to set that at.

I could go with the "content--wide" width now set for art on the interior pages.

@choldgraf
Copy link
Collaborator

Good point - your proposed solution sounds good to me!

@palewire
Copy link
Collaborator Author

palewire commented Jan 9, 2022

The max width is in. Here's what you see now.

Screenshot from 2022-01-09 11-12-18

Screenshot from 2022-01-09 11-12-30

@krassowski
Copy link
Member

For the lower part, could we use white text color? C-f #587

@choldgraf
Copy link
Collaborator

+1 from me - it does makes sense that white would be more accessible

@choldgraf
Copy link
Collaborator

@palewire what do you think about the proposal to have white text in the footer?

@palewire
Copy link
Collaborator Author

palewire commented Jan 11, 2022

Was hoping to put in sometime mocking up alternatives and then getting them tested, not just by Lighthouse, but also by the alternative standard mentioned in the issue. I haven't made time for that yet. My hope would be that with a little experimentation we could use that method to settle on one color that would work in both sections of the footer, just for consistency's sake. Maybe I'm overthinking it.

@choldgraf
Copy link
Collaborator

sounds good - no hurry, just wanted to make sure you saw the conversation. Want to do that before finishing this PR? Or open an issue to focus on the footer color and add it to the milestone?

@palewire
Copy link
Collaborator Author

I'm okay either way. I just want to make sure we ultimately nail down a solid approach we can point to standards with. That can be now, or it can be later.

@choldgraf
Copy link
Collaborator

@krassowski would you mind if we tracked the footer text color in a dedicated issue? I think the addition of the links here is a nice addition, so I'm inclined to move forward in the name of iterative progress, but what do you think?

@krassowski
Copy link
Member

I'm fine with that :). I would suggest a rebase/merge of master before merging into matter to double check that validator and links check passes.

@palewire
Copy link
Collaborator Author

Master is rebased

Copy link
Collaborator

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

Awesome - thanks for this improvement!

@choldgraf choldgraf merged commit 68f2fde into master Jan 11, 2022
@choldgraf choldgraf deleted the bigger-footer branch January 11, 2022 22:42
@choldgraf
Copy link
Collaborator

choldgraf commented Jan 11, 2022

hmmm the footer link list seems a bit wonky. This is on a wide screen:

image

@choldgraf
Copy link
Collaborator

(note that I didn't see this in the PR preview, not sure why the behavior is different)

@palewire
Copy link
Collaborator Author

I suspect this is a caching issue that will resolve once the TTL expires. But I could be wrong.

@choldgraf
Copy link
Collaborator

yep you're right, it's fixed now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expand the footer to include two link lists, one for nav items and a second for project pages
3 participants