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

Desktop theme Card #4140

Merged
merged 2 commits into from
Oct 30, 2023
Merged

Desktop theme Card #4140

merged 2 commits into from
Oct 30, 2023

Conversation

jonahkagan
Copy link
Collaborator

@jonahkagan jonahkagan commented Oct 30, 2023

Overview

Adapts the Card component for use in desktop apps. Currently it was only used in one place: the VxMark review screen, so it was a pretty easy change.

Then replaces the makeshift Card component in VxDesign with the libs/ui component.

Review by commit

Demo Video or Screenshot

Before
Screenshot-VxMark-2023-10-30T18:22:08 171Z

After
Screenshot-VxMark-2023-10-30T18:21:09 767Z

Testing Plan

Automated tests, visual confirmation

Checklist

  • I have added logging where appropriate to any new user actions, system updates such as file reads or storage writes, or errors introduced.
  • I have added a screenshot and/or video to this PR to demo the change
  • I have added the "user_facing_change" label to this PR to automate an announcement in #machine-product-updates

- Remove elevation effect
- Remove margin
- Add style prop
@jonahkagan jonahkagan marked this pull request as ready for review October 30, 2023 18:24
@jonahkagan jonahkagan requested a review from a team as a code owner October 30, 2023 18:24
@jonahkagan jonahkagan requested review from eventualbuddha and removed request for a team and eventualbuddha October 30, 2023 18:24
Copy link
Collaborator

@adghayes adghayes left a comment

Choose a reason for hiding this comment

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

What's the rationale for removing the box shadow / elevation effect?

@jonahkagan
Copy link
Collaborator Author

jonahkagan commented Oct 30, 2023

What's the rationale for removing the box shadow / elevation effect?

We don't really use elevation to group things anywhere else in our apps, so it felt inconsistent. This became especially apparent in the desktop theme.

Copy link
Collaborator

@adghayes adghayes left a comment

Choose a reason for hiding this comment

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

SGTM

@jonahkagan jonahkagan merged commit 07f52cd into main Oct 30, 2023
50 checks passed
@jonahkagan jonahkagan deleted the jonah/desktop-card branch October 30, 2023 19:41
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.

2 participants