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

chore(sidebarwidgets): refresh styles #1841

Merged
merged 7 commits into from
Nov 5, 2024
Merged

chore(sidebarwidgets): refresh styles #1841

merged 7 commits into from
Nov 5, 2024

Conversation

dancormier
Copy link
Contributor

@dancormier dancormier commented Oct 14, 2024 •
edited
Loading

STACKS-673

This PR refreshes styles for the Stacks Sidebar Widget component, per the Figma.

How to test

Tip

Netlify deploy previews are not working right now, so you'll need to run this locally to see the changes
Dev preview

  1. Visit the Dev preview
    • To test locally, checkout out this branch (STACKS-673/sidebarwidget-style-refresh) locally and run it (npm i && npm run start), head to the sidebar widgets docs page to see the changes

Screenshots

Basic

Before

image

After

image

Navigation

Before

image

After

image

Header actions

Before

image

After

image

Accordion

Before

image

After

image

@dancormier dancormier marked this pull request as ready for review October 28, 2024 21:34
giamir
giamir approved these changes Oct 29, 2024
View reviewed changes
Copy link
Contributor

@giamir giamir left a comment

Choose a reason for hiding this comment

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

Great work @dancormier. Code changes make sense to me. I found only a small issue with a variant of the visual tests (see comment).

screenshots/Chromium/baseline/s-sidebarwidget-dark-blue-ws3-small-bold-headers.png
Copy link
Contributor

Choose a reason for hiding this comment

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

small-bold-headers screenshots look odd. We might need to add a s-sidebarwidget--content sibling element in the test setup because the header element by itself behave strangely not leaving enough padding at the bottom. This is probably because you are not supposed to use a header element in isolation.

@giamir
Copy link
Contributor

giamir commented Oct 29, 2024

Netlify deploy previews are not working right now, so you'll need to run this locally to see the changes

@dancormier I have spent some time trying to fix this but unfortunately it doesn't look netlify is honoring the GIT_LFS_SKIP_SMUDGE variable nor the GIT_LFS_FETCH_INCLUDE which they should according to their docs.

I talked to @abovedave and as a first step we can reach out to them for support via email.
Worst case scenario we can move to use their CLI for direct deployments in github workflows (I did a test and it works) instead of using the GH repo linked integration. The downside of doing that is that we won't have the netlify bot commenting on PRs, etc... We could achieve something similar also by expanding the GH workflow we would create but it would be nice if we could just stick to their linked repo option. I will draft a support email and cc you.

@netlify Netlify
Copy link

netlify bot commented Oct 29, 2024 •
edited
Loading

❌ Deploy Preview for stacks failed. Why did it fail? →

Name Link
🔨 Latest commit 3bd902b
🔍 Latest deploy log https://app.netlify.com/sites/stacks/deploys/672145166fcc5e00084236cb

CGuindon
CGuindon approved these changes Oct 31, 2024
View reviewed changes
Copy link
Collaborator

@CGuindon CGuindon left a comment

Choose a reason for hiding this comment

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

LGTM but would still wait for a check from @andrewmeacham

andrewmeacham
andrewmeacham approved these changes Oct 31, 2024
View reviewed changes
Copy link
Contributor

@andrewmeacham andrewmeacham left a comment

Choose a reason for hiding this comment

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

Looks great!

@dancormier dancormier merged commit 4c540fa into develop Nov 5, 2024
7 of 11 checks passed
@dancormier dancormier deleted the STACKS-673/sidebarwidget-style-refresh branch November 5, 2024 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@giamir giamir giamir approved these changes

@CGuindon CGuindon CGuindon approved these changes

@andrewmeacham andrewmeacham andrewmeacham approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dancormier @giamir @andrewmeacham @CGuindon