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

docs(website): style tweaks for readability and more open spacing #8876

Merged
merged 10 commits into from
Sep 26, 2023

Conversation

jeffmerrick
Copy link
Contributor

Many style tweaks for readability and more open spacing. Tried to use theme variables wherever possible.

  • Manrope removed in favor of system fonts for body copy - now only used in headings and in bold.
  • Global font size reduced by 5% to give everything a bit more room.
  • Spacing generally increased, tweaks to sidebar menu and TOC.
  • New sidebar menu active styles.
  • Light organization/cleanup of global.scss

Screen Shot 2023-09-21 at 8 44 17 AM

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
  • For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

Copy link
Collaborator

@hsheth2 hsheth2 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 great already - a huge improvement over our existing styles

I do feel like the sidebar has a bit too much left/right padding, but we can tweak that in a follow-up

There is a merge conflict which will need to get fixed before we can merge this

@hsheth2
Copy link
Collaborator

hsheth2 commented Sep 22, 2023

@jeffmerrick the CTAs look a bit bulky now - is this what you intended?

image

@jeffmerrick
Copy link
Contributor Author

@hsheth2 Ah nope must have been the high --ifm-global-spacing I set. I've made it lower and we can adjust other ones as needed. (Not sure how much of an effect it had on other elements anyhow.)

@hsheth2
Copy link
Collaborator

hsheth2 commented Sep 23, 2023

@jeffmerrick some additional feedback - I think it make sense to fix in this PR instead of in a follow-up

The font weight on the cards feels a bit too heavy here. Also the text looks slightly misaligned with the card header text
image

The sidebar feels too wide - I think 350px is a bit too much, and we should probably also reduce the left and right padding on the sidebar items

Finally, the min left and right padding on the main content feels a bit low on smaller screens (as in the screenshot)
image

cc @yoonhyejin - lmk if you disagree with any of this / want to tweak other things

@yoonhyejin
Copy link
Collaborator

Generally agree with @hsheth2 , especially on the left/right padding of the main content

@jeffmerrick
Copy link
Contributor Author

No problem, made these changes:

  • Sidebar width changed back to default
  • Sidebar padding changed back to default
  • Padded main content area independently of other variables (was affected by the change to --ifm-global-spacing)

Can you explain more what you mean on:

The font weight on the cards feels a bit too heavy here. Also the text looks slightly misaligned with the card header text

Alignment looks ok to me and font weight/size matches the lists that follow.

@hsheth2
Copy link
Collaborator

hsheth2 commented Sep 25, 2023

Two screenshots, first the tweaked version and second from the existing one.

The font weight on the top cards (e.g. "Get Started", "Ingest Metadata") and the cards below (e.g. "Managed DataHub", "Docker") looks heavier than before, and it felt a bit out of place to me. The headers (e.g. "Deployment", "Deployment Guide") look good though.

On second look, I agree with you that the alignment is fine though.

image image

@jeffmerrick
Copy link
Contributor Author

I think previously we were going only up to semibold for font weight and now we go up to bold. (Also those are now just the default system font instead of Manrope). We can pull back the weight, here are a few options:

  1. Just the global bold variable changed to semibold (will affect bold everywhere except headings):

Screen Shot 2023-09-25 at 4 46 56 PM

  1. Both global bold and headings changed to semibold:

Screen Shot 2023-09-25 at 4 44 28 PM

  1. We could also just change the weight on those cards, but it would be the only place we're using semibold.

@yoonhyejin
Copy link
Collaborator

I prefer the first one here, seems much better.

@hsheth2
Copy link
Collaborator

hsheth2 commented Sep 26, 2023

@jeffmerrick fwiw it looks like we're also using semibold for --ifm-button-font-weight

I'd say we should change the font-weight on those cards, so it looks like option 1 without changing the global bold font weight

@jeffmerrick
Copy link
Contributor Author

Yep, you're right, it's used for buttons too. I've made this change along with making the lists underneath semibold for consistency.

@hsheth2 hsheth2 merged commit f95d1ae into datahub-project:master Sep 26, 2023
36 of 37 checks passed
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.

3 participants