Skip to content

Home #107

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 26 commits into from
Aug 24, 2023
Merged

Home #107

merged 26 commits into from
Aug 24, 2023

Conversation

machikoyasuda
Copy link
Member

@machikoyasuda machikoyasuda commented Aug 21, 2023

fix #65

Note: This PR was created off of fix/footer #106. These 2 changes can go out together in sync, so the Home page doesn't look disjointed.

What this PR does

- Home page: Adds new home headline, paragraph text, image. Upload new brandmark. New cyan/teal color.
- Header: Removes the large logo logic. Add the box-shadow.
- Whole app: Set correct background color

Components

  • Configure btn-primary, btn-outline-primary with :hover, :focus, :disabled modes all designed.
  • Configure .h2 h2 and .h3. The home page has a h1 that uses a .h2 style class. The job posting page uses a .h3 class style on its h2s. This PR does not re-do all the headline font sizes. That will come in a separate PR Fix: Font weights #116
  • Sets focus ring for buttons and links.
  • Set text-info to be gray color.

Screenshots

Home

image image image

Jobs (Ensure no regressions)

image

Posting (Ensure no regressions)

image image Has the new primary disabled button

Perf / A11y

image image

@machikoyasuda machikoyasuda self-assigned this Aug 22, 2023
@machikoyasuda
Copy link
Member Author

machikoyasuda commented Aug 22, 2023

Ready for review. Should be reviewed after #106, but can (and probably should) be merged at the same time. Just have 1 outstanding question:

@thekaveman The design calls for removing the Capabilities Statement button. But since we don't have the #44 yet, we don't have any other way for people to get to it. Should I keep the button there? Should I remove the Get in touch button, so at least the design is the same with 1 button? There is a contact email link right below it.

@segacy1 If we do need 2 buttons, what should the 2nd button look like? I guessed what an outline button might look like, but I'm not sure about how it looks on hover.

@machikoyasuda machikoyasuda marked this pull request as ready for review August 22, 2023 00:51
@machikoyasuda machikoyasuda requested a review from a team as a code owner August 22, 2023 00:51
@machikoyasuda machikoyasuda linked an issue Aug 22, 2023 that may be closed by this pull request
@segacy1
Copy link

segacy1 commented Aug 22, 2023

Created a version of the homepage with both buttons in figma too so we have it here

For the secondary button hover, updated to this:
Screenshot 2023-08-22 at 11 49 37 AM

In Figma here

Also, two additional notes:

  1. For the focus states for both primary and secondary buttons styles, I've updated the distance between the button and the focus border to be 4px
  2. I've changed all the secondary button borders to 1.5px width - it was looking a little too thin to me

@machikoyasuda
Copy link
Member Author

Marking this back as In Progress as I update the outline button.

@machikoyasuda machikoyasuda marked this pull request as draft August 22, 2023 16:38
@machikoyasuda
Copy link
Member Author

@segacy1
image
Neither of these disabled buttons have sufficient color contrast.

It won't affect the home page, but it will affect the job post page. The job post page has a disabled button when the job is no longer open, like this: https://compiler.la/jobs/executive-assistant

image
 • Error: This element has insufficient contrast at this conformance level. Expected a contrast ratio of at least 4.5:1, but text in this element has a contrast ratio of 4.3:1. Recommendation:  change text colour to #171717.
   ├── WCAG2AA.Principle1.Guideline1_4.1_4_3.G18.Fail
   ├── #apply
   └── <a class="d-inline-block monospace btn btn-primary disabled" id="apply" href="https://forms.clickup.com/8631512/f/87d6r-2087/GUOF0LDD4SN58BC27T"> Apply Now </a>

 • Error: This element has insufficient contrast at this conformance level. Expected a contrast ratio of at least 4.5:1, but text in this element has a contrast ratio of 4.3:1. Recommendation:  change text colour to #171717.
   ├── WCAG2AA.Principle1.Guideline1_4.1_4_3.G18.Fail
   ├── html > body > main > div > div > div:nth-child(27) > a:nth-child(1)
   └── <a class="disabled monospace btn btn-primary" target="_blank" href="https://github.com/capabilities">Capabilities Statement</a>

 • Error: This element has insufficient contrast at this conformance level. Expected a contrast ratio of at least 4.5:1, but text in this element has a contrast ratio of 4.3:1. Recommendation:  change background to #171717.
   ├── WCAG2AA.Principle1.Guideline1_4.1_4_3.G18.Fail
   ├── html > body > main > div > div > div:nth-child(27) > a:nth-child(2)
   └── <a class="disabled monospace btn btn-outline-primary" href="mailto:[email protected]">Get in touch</a>

3 Errors

--bs-font-sans-serif: "Roboto", system-ui,-apple-system,"Segoe UI","Helvetica Neue","Noto Sans","Liberation Sans",Arial,sans-serif,"Apple Color Emoji","Segoe UI Emoji","Segoe UI Symbol","Noto Color Emoji";
--bs-font-monospace: "Source Code Pro Bold",monospace;
--focus-box-shadow: 0 0 0 calc(2.5rem / 16) var(--brand-primary-black), 0 0 0 calc(4rem / 16) var(--brand-primary-green);
Copy link
Member Author

Choose a reason for hiding this comment

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

This code creates this effect as a box-shadow:
image

How to read this code:

  • 0 0 0 calc(2.5rem / 16) var(--brand-primary-black): Create a 2.5px-thick black line, the same color as the background color.
  • 0 0 0 calc(4rem / 16) var(--brand-primary-green): Below that first line, starting from the same point, add another 4px of green line. Only 1.5px of green will actually show up. That's calculated like this: 4-2.5 = 1.5.

Comment on lines -47 to -53
li>a {
text-decoration: none;
Copy link
Member Author

Choose a reason for hiding this comment

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

No longer necessary because all a elements have text-decoration: none now.

Comment on lines 115 to 126
h1 {
font-size: 2rem;
margin-top: 1.5rem;
margin-bottom: 1.5rem;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Updating h1, h4 will happen in subsequent PR #37. It will be required for completing #66.

h3 {
font-size: 1.17rem;
h3,
.posting h2 {
Copy link
Member Author

Choose a reason for hiding this comment

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

Since each Job Post is created from Markdown, and we can't declare CSS classes in Markdown, this .posting h2 declares that all H2s on a job post should actually use h3 styles, which is what Figma calls for.

Comment on lines +216 to +228
.btn-primary:focus,
.btn-primary:focus-visible,
.btn-outline-primary:focus,
.btn-outline-primary:focus-visible {
background-color: var(--bs-btn-bg);
border-color: var(--bs-btn-border-color);
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a really, really neat nesting CSS Variable feature.

The underlying Figma logic calls for: For btn-primary and btn-outline-primary, the background color on focus should be the same as the default background color. Same with the border color. Bootstrap code has these values set to the hover colors instead - which is brighter - instead of the default color.

In the design, the background color for Primary is green, and the background color for Outline is black.

In the code, this is how those values are set:

.btn-primary {
    --bs-btn-bg: var(--button-primary-default);
    --bs-btn-border-color: var(--button-primary-default);
}

.btn-outline-primary {
    --bs-btn-bg: transparent;
    --bs-btn-border-color: var(--button-primary-default);

In this code below, I'm able to set the background-color to be the var(--bs-btn-bg) in each class's context. Even though var(--bs-btn-bg) is set to different values for .btn-primary and .btn-outline-primary, the CSS figures out what value is being scoped. Saves time and lines of code.

.btn-primary:focus,
.btn-primary:focus-visible,
.btn-outline-primary:focus,
.btn-outline-primary:focus-visible {
    background-color: var(--bs-btn-bg);
    border-color: var(--bs-btn-border-color);
}

@machikoyasuda machikoyasuda marked this pull request as ready for review August 22, 2023 23:29
@machikoyasuda
Copy link
Member Author

Added a bunch of explanatory comments.

@thekaveman @angela-tran Ready for review.

Ideally, both this PR and the #106 PR can go out at the same time, so that the Home Page looks in sync with the new Footer.

<h2 class="mt-5 fs-6">Past roles</h2>
<ul id="closed" class="list-unstyled"></ul>
<h2 class="mt-5 fs-6 pb-1 mb-2">Past roles</h2>
<ul id="closed" class="list-unstyled d-flex flex-column gap-2"></ul>
Copy link
Member Author

Choose a reason for hiding this comment

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

Use d-flex flex-column gap-2 to create a 8px vertical gap (as in, padding) between the list items. d-flex flex-column sets the links to flow vertically, rather than horizontally and tells gap to create the gap vertically.

@thekaveman
Copy link
Member

@machikoyasuda

The design calls for removing the Capabilities Statement button. But since we don't have the #44 yet, we don't have any other way for people to get to it. Should I keep the button there?

Let's leave both buttons there for now, until we have the nav from #44.

/cc @AnthonyRollins

@machikoyasuda machikoyasuda mentioned this pull request Aug 23, 2023
Base automatically changed from fix/footer to main August 23, 2023 00:40
@netlify
Copy link

netlify bot commented Aug 23, 2023

Deploy Preview for compilerla ready!

Name Link
🔨 Latest commit b316b50
🔍 Latest deploy log https://app.netlify.com/sites/compilerla/deploys/64e7bbbae7c0d5000937c532
😎 Deploy Preview https://deploy-preview-107--compilerla.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@machikoyasuda
Copy link
Member Author

machikoyasuda commented Aug 23, 2023

@segacy1 There's a Netlify preview link now, so you can check out how the buttons look: https://deploy-preview-107--compilerla.netlify.app/

@machikoyasuda
Copy link
Member Author

Funny mobile browser quirk: Mobile Safari thinks these codes are phone numbers

F1CC815F-3136-4DDB-86A9-A21E9608CA7F
Uploading 6DC4A245-0A45-477D-9BCE-D6414B5F37C0.png…

@machikoyasuda
Copy link
Member Author

Rebased with latest main

@segacy1
Copy link

segacy1 commented Aug 23, 2023

Hi @machikoyasuda - 2 quick notes from reviewing on Netlify

  1. Could we remove the drop shadow from the nav?
  2. Could we vertically center the content on the page between nav and footer?

I'll make sure that's reflected in Figma as well

@machikoyasuda
Copy link
Member Author

@segacy1 These 2 fixes are now out here: https://deploy-preview-107--compilerla.netlify.app/

@machikoyasuda machikoyasuda requested a review from segacy1 August 23, 2023 19:32
Copy link
Member

Choose a reason for hiding this comment

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

Can we please avoid using compiler-la as an ID or engineering string whenever possible? Either compiler or compiler-llc if you need a longer version. For this is could even be like brand-pattern or shapes-brand-pattern or something.

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it. I put the company name in there for SEO reasons 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

But Compiler LA is not the official company name anymore right? It's Compiler LLC.

Copy link
Member Author

Choose a reason for hiding this comment

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

renamed 52b9c66

Copy link
Member Author

Choose a reason for hiding this comment

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

@thekaveman need a re-review for this! thank u

@machikoyasuda machikoyasuda mentioned this pull request Aug 23, 2023
@machikoyasuda machikoyasuda added this to the Home Page milestone Aug 23, 2023
Copy link
Member

@angela-tran angela-tran left a comment

Choose a reason for hiding this comment

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

The focus ring is such a simple thing, yet makes the site feel more polished! I like it.

I noticed the focus ring around our logo is a little weird - is that an easy fix?
image

@machikoyasuda
Copy link
Member Author

@angela-tran 615c4bb

image

Works just by adding d-inline-block to the parent a tag.

Copy link
Member

@angela-tran angela-tran left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@machikoyasuda machikoyasuda merged commit 66b0481 into main Aug 24, 2023
@machikoyasuda machikoyasuda deleted the feat/65-new-home branch August 24, 2023 21:41
angela-tran added a commit that referenced this pull request Jan 24, 2024
#107 changed the class names for the button styles
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Development work for Home page
4 participants