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

1165/design items #1270

Merged
merged 9 commits into from
May 23, 2022
Merged

1165/design items #1270

merged 9 commits into from
May 23, 2022

Conversation

adriencyberspace
Copy link

@adriencyberspace adriencyberspace commented May 10, 2022

Issue #1165

Description

  1. Make max width and margins for language nav the same as the navbar
  2. Footer: we should increase space between listing questions and general questions to 2rem
  3. Footer-section component with logo and contact content should have max width of 1024px so text doesn't run too long
  4. Menu icon not left aligned between md and lg screen widths
  5. Should have 1px line between the header and the content of the single listing page
  6. Tooltip has line between bubble and arrow (see below)

How Can This Be Tested/Reviewed?

Entry Point 1: http://localhost:3000/ | https://detroit-public-dev.netlify.app/

  1. View header - language nav width and padding the same as navbar width and padding (Note: the border around the Sign Up button and lack of border around the right-most language button create the appearance that they are not aligned)
  2. View footer - there should be a 2rem margin below listing questions section
  3. Still on footer - left footer section should have a max-width of 1024px
  4. Narrow screen width to between 768px and 1024px - the hamburger menu icon should remain right-aligned

Entry Point 2: http://localhost:3000/listings | https://detroit-public-dev.netlify.app/listings

  1. Click into any listing - notice a 1px line between the Site Header and the content of the page (Note: I removed the top border of the process container to avoid clashing with the new divider line)
  2. Still on listing page, click the tooltip - there should be no 1px border between the tooltip main rectangle and the tooltip arrow

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have reviewed the changes in a desktop view
  • I have reviewed the changes in a mobile view
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have assigned reviewers
  • I have run yarn generate:client and/or created a migration if I made backend changes that require them
  • I have exported any new pieces added to ui-components
  • My commit message(s) is/are polished, and any breaking changes are indicated in the message and are well-described
  • Commits made across packages purposefully have the same commit message/version change, else are separated into different commits

Reviewer Notes:

Steps to review a PR:

  • Read and understand the issue
  • Review the code itself from a style point of view
  • Pull the changes down locally and test that the acceptance criteria is met
  • Also review the acceptance criteria on the Netlify deploy preview (noting that these do not yet include any backend changes made in the PR)
  • Either explicitly ask a clarifying question, request changes, or approve the PR if there are small remaining changes but the PR is otherwise good to go

On Merge:

If you have one commit and message, squash. If you need each message to be applied, rebase and merge.

@netlify
Copy link

netlify bot commented May 10, 2022

Deploy Preview for detroit-public-dev ready!

Name Link
🔨 Latest commit e14bc20
🔍 Latest deploy log https://app.netlify.com/sites/detroit-public-dev/deploys/628ba2f401909b000943b2fb
😎 Deploy Preview https://deploy-preview-1270--detroit-public-dev.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 settings.

@netlify
Copy link

netlify bot commented May 10, 2022

Deploy Preview for detroit-partners-dev ready!

Name Link
🔨 Latest commit e14bc20
🔍 Latest deploy log https://app.netlify.com/sites/detroit-partners-dev/deploys/628ba2f4ef472300083f2060
😎 Deploy Preview https://deploy-preview-1270--detroit-partners-dev.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 settings.

Copy link
Collaborator

@emilyjablonski emilyjablonski left a comment

Choose a reason for hiding this comment

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

I'm not seeing: Footer-section component with logo and contact content should have max width of 1024px so text doesn't run too long
Let's remove this one on mobile only: Should have 1px line between the header and the content of the single listing page

@slowbot
Copy link
Collaborator

slowbot commented May 12, 2022

@adriencyberspace

  • Content of the footer should have a max width of 1024 and be centered.

Copy link
Collaborator

@YazeedLoonat YazeedLoonat left a comment

Choose a reason for hiding this comment

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

a lot of this looks good, one thing I'm noticing (and this could be my 👀 or my computer) but it seems like there is still a line on tooltips that cuts really close to the end of the tooltip

can you take a look at that and see if you can resolve please!

Copy link
Collaborator

@YazeedLoonat YazeedLoonat left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@seanmalbert seanmalbert left a comment

Choose a reason for hiding this comment

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

This looks good to me! Since we'll wait for @emilyjablonski's approval of the changes until she's back, I'm going to move it to QA with the preview in the meantime.

@adriencyberspace adriencyberspace merged commit fc8bc77 into dev May 23, 2022
@adriencyberspace adriencyberspace deleted the 1165/design-fixes branch May 23, 2022 15:37
seanmalbert added a commit that referenced this pull request Jun 23, 2022
* style: issues 1, 2, 3, 5, 6

* style: issue 4 divider line

* feat: ListingDividerLine

* feat: reset tooltip functionality

* style: footer-sections div max width 1024px, centered

* style: remove divider line on mobile

* style: make tooltip taller but overlap more so it displays the same but no line shows

Co-authored-by: Sean Albert <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Detroit - Follow-up Visual Design Items
5 participants