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.

Sign up for GitHub

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

Jump to bottom

Add Brand Guide to repo #26

Merged
merged 17 commits into from
Jul 28, 2023
Merged

Add Brand Guide to repo #26

merged 17 commits into from
Jul 28, 2023

Conversation

geekygirlsarah
Copy link
Contributor

@geekygirlsarah geekygirlsarah commented Jul 13, 2023 •
edited

Closes https://github.com/18F/TLC-crew/issues/286

Changes proposed in this pull request:

This adds in the Brand Guide.

  • Adds title to the header above navigation
  • Adds in main navigation links on top menu bar (no side bar menus)
  • Adds in all content pages
  • Adds in images and downloads it references
  • Fixes some (maybe not all?) {% include... %} links
  • Doesn't add in shared GSA/TTS/18F footers

WIP:

  • Attribute tags don't fully work yet (which causes a build error)

Since I'm leaving the org, you can build off this PR or close it and start a new one!

@geekygirlsarah
(wip) Add Brand guide
d3cf511
@geekygirlsarah
Merge branch 'main' into sw/add-brand-guide
1f67593 
# Conflicts:
#	_data/navigation.yaml
#	_data/titles_roots.yaml
#	_includes/guidelist.html
@geekygirlsarah
(wip) Fix Brand menu issue, fix rest of front matter issues
b3d9972
@geekygirlsarah
(wip) Add Brand guide images/assets
26e655a
@geekygirlsarah
Merge branch 'main' into sw/add-brand-guide
4c66a53 
# Conflicts:
#	_data/navigation.yaml
#	_data/titles_roots.yaml
#	_includes/guidelist.html
@beechnut
Fix color matrix content
a74ed49 
Note that this commit doesn't fix the styling, just the content on the color matrix page

- Adds filters contrastRatio and humanReadableContrastRatio to calculate ratios for display
- Adds markdown-it-attrs to allow for inline styling of Markdown links {:.usa-button}
- Documents how to add an npm package
- Deletes test files
@beechnut
Reorganize images into the right place
b85effa
@beechnut
Add files to dist, and get images loading
0495f41
@beechnut
Change configuration to let us point to a download path
cdb6876
@beechnut
Move color matrix closer to the brand content, instead of in the root…
dcae8d9 
… _includes
@beechnut
Add non-working styles, fix downloads
6806d85 
- Copies all style files from the brand guide, but does not link them: styles are still broken in complex ways
- Changes download paths, to make downloading files work
@beechnut beechnut changed the title [Draft] Add Brand Guide to repo Add Brand Guide to repo Jul 25, 2023
@beechnut beechnut marked this pull request as ready for review July 25, 2023 16:43
@beechnut beechnut requested review from alexbielen, MelissaBraxton and igorkorenfeld and removed request for beechnut July 25, 2023 16:44
beechnut
beechnut reviewed Jul 25, 2023
View reviewed changes
.eleventy.js Show resolved Hide resolved
beechnut
beechnut reviewed Jul 25, 2023
View reviewed changes
.eleventy.js Outdated Show resolved Hide resolved
igorkorenfeld
igorkorenfeld requested changes Jul 25, 2023
View reviewed changes
Copy link
Member

@igorkorenfeld igorkorenfeld left a comment

Choose a reason for hiding this comment

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

Thanks for pushing this so far along, it's looking nice! Just a few changes to make before it's 👍

_data/titles_roots.yaml Outdated Show resolved Hide resolved
content/brand/logo.md Outdated Show resolved Hide resolved
content/brand/logo.md Outdated Show resolved Hide resolved
assets/brand/styles/_color-matrix-old.scss Outdated Show resolved Hide resolved
.eleventy.js Show resolved Hide resolved
.eleventy.js Outdated Show resolved Hide resolved
content/brand/color-palette.md Show resolved Hide resolved
content/brand/color-palette.md Outdated Show resolved Hide resolved
content/brand/typography.md Show resolved Hide resolved
@beechnut
Respond to code review
3754bcc 
- Update Guide title to "Visual identity"
- Delete -old files
- Move filter functions, including color contrast, to config/
- Fix line breaks, styles, and URLs across pages
- Add note to README about how to link files for download
@beechnut beechnut self-assigned this Jul 26, 2023
igorkorenfeld
igorkorenfeld approved these changes Jul 26, 2023
View reviewed changes
Copy link
Member

@igorkorenfeld igorkorenfeld left a comment

Choose a reason for hiding this comment

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

Just one broken download link to fix, otherwise looks good to me. Thanks for making all of the changes!

Would you mind making a ticket to capture the follow-on style work for this one?

content/brand/logo.md Outdated Show resolved Hide resolved
@beechnut
Copy link
Contributor

I just double-checked, all of the failing HTML validation tests and pa11y issues are happening in the Engineering Guide. I'd like someone's approval (@alexbielen? @igorkorenfeld?) to merge, given that context.

@igorkorenfeld
Copy link
Member

It looks like one of the pa11y flags is on the color-matrix table (copied below) is that something we could fix before merging? Or could that be put on a separate branch so it doesn't get flagged for other PRs.

I just realized that we should also update the description for the PR since it has changed from the original WIP.

Errors in http://localhost:8080/content/brand/_includes/color-matrix/:

 • A title should be provided for the document, using a non-empty title element
   in the head section.

   (html > head)

   <head></head>

 • The html element should have a lang or xml:lang attribute which describes
   the language of the document.

   (html)

   <html><head></head><body><svg class="...</html>

@beechnut
Copy link
Contributor

@igorkorenfeld Somehow I didn't see that one, thanks for catching! But since that page is an include, my understanding is that having a second <head> or title element would be incorrect both for accessibility and for HTML validation.

Looking deeper: I'm noticing that it's scanning http://localhost:8080/content/brand/_includes/color-matrix/, which strikes me as an error. It should be scanning /_site to assess the finished product, not the source folders, right?

Looking even deeper: Since pa11y scans sitemap.xml, this also implies that the sitemap is generating incorrect data — there's no finished site whose URLs should contain a path like /content/brand/_includes.

The more I dig in, the more this strikes me as out of scope for this ticket. Thoughts?

@igorkorenfeld
Copy link
Member

Ah that's a great point! Eleventy is processing that file as a page, and it shouldn't. I believe if this file lived within the main _includes folder (can still be in the brand subfolder within it), 11ty will ignore it and not try to output it. If it works the way I think it does, that should stop it from being included in the sitemap, and from pa11y scanning it.

@kewlguy781
Copy link
Contributor

The engineering part that is failing in pa11y testing has been fixed in main. pull the main and it should pass the checks.

@beechnut beechnut merged commit f4cf3a2 into main Jul 28, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@beechnut beechnut beechnut left review comments

@igorkorenfeld igorkorenfeld igorkorenfeld approved these changes

@alexbielen alexbielen Awaiting requested review from alexbielen

@MelissaBraxton MelissaBraxton Awaiting requested review from MelissaBraxton

Assignees

@beechnut beechnut

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@geekygirlsarah @beechnut @igorkorenfeld @kewlguy781