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

Add spacing tokens and use in settings view #7200

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

olmoh
Copy link
Collaborator

@olmoh olmoh commented Nov 19, 2024

Adds spacing tokens based on design foundations and updates the settings view to use these


This change is Reviewable

@olmoh olmoh requested a review from raksooo November 19, 2024 14:19
Copy link

linear bot commented Nov 19, 2024

@olmoh olmoh force-pushed the change-to-new-spacings-in-settings-view-des-1454 branch 2 times, most recently from d6d8433 to 2fdfd8e Compare November 20, 2024 11:00
Copy link
Member

@raksooo raksooo left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, 4 of 7 files at r2, 5 of 6 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @olmoh)


desktop/packages/mullvad-vpn/src/renderer/components/SettingsHeader.tsx line 9 at r4 (raw file):

export const Container = styled.div({
  flex: 0,
  margin: `${spacings.spacing3} ${spacings.spacing5} ${spacings.spacing4}`,

I looked around a bit in the designs, and it looks like the default horizontal view margin is spacing5. Maybe we should split it up into two variables, one for horizontal view margin and one for vertical? Then we could use the horizontal one here. What do you think?


desktop/packages/mullvad-vpn/src/renderer/components/SettingsStyles.tsx line 12 at r4 (raw file):

export const StyledQuitButton = styled(AppButton.RedButton)({
  margin: `0 ${measurements.viewMargin}`,

I'm a bit unsure about using viewMargin as marginTop since that corresponds to the outer margin of the view. What do you think about using viewMargin for the bottom margin and the corresponding spacing value for marginTop? If we ever want to change the view margin without changing the margin between the button and the settings cells it would be easier to accomplish that.


desktop/packages/mullvad-vpn/src/renderer/components/Layout.tsx line 54 at r4 (raw file):

  flex: 0,
  padding: measurements.viewMargin,
  [`${SettingsContent} &`]: {

I think you want && here since applying styles to & can lead to unintended behavior.

@olmoh olmoh force-pushed the change-to-new-spacings-in-settings-view-des-1454 branch from 2fdfd8e to 2a13e68 Compare November 21, 2024 08:50
@olmoh olmoh requested a review from raksooo November 21, 2024 08:55
Copy link
Collaborator Author

@olmoh olmoh left a comment

Choose a reason for hiding this comment

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

Reviewable status: 7 of 28 files reviewed, 2 unresolved discussions (waiting on @raksooo)


desktop/packages/mullvad-vpn/src/renderer/components/Layout.tsx line 54 at r4 (raw file):

Previously, raksooo (Oskar Nyberg) wrote…

I think you want && here since applying styles to & can lead to unintended behavior.

Done.


desktop/packages/mullvad-vpn/src/renderer/components/SettingsHeader.tsx line 9 at r4 (raw file):

Previously, raksooo (Oskar Nyberg) wrote…

I looked around a bit in the designs, and it looks like the default horizontal view margin is spacing5. Maybe we should split it up into two variables, one for horizontal view margin and one for vertical? Then we could use the horizontal one here. What do you think?

Good idea, I split up view margin into a horizontal and a vertical variant and changed all uses of it so the horizontal is only used on horizontal spacing.

@olmoh olmoh force-pushed the change-to-new-spacings-in-settings-view-des-1454 branch from 2a13e68 to 9157302 Compare November 21, 2024 14:19
@olmoh olmoh force-pushed the change-to-new-spacings-in-settings-view-des-1454 branch from 9157302 to cefb9c4 Compare November 21, 2024 14:20
Copy link
Member

@raksooo raksooo left a comment

Choose a reason for hiding this comment

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

Reviewed 17 of 21 files at r5, 5 of 5 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @olmoh)


desktop/packages/mullvad-vpn/src/renderer/components/Layout.tsx line 54 at r4 (raw file):

Previously, olmoh (Oliver Mohlin) wrote…

Done.

Great! 👍


desktop/packages/mullvad-vpn/src/renderer/components/SettingsHeader.tsx line 9 at r4 (raw file):

Previously, olmoh (Oliver Mohlin) wrote…

Good idea, I split up view margin into a horizontal and a vertical variant and changed all uses of it so the horizontal is only used on horizontal spacing.

Nice work!


desktop/packages/mullvad-vpn/src/renderer/components/Layout.tsx line 59 at r6 (raw file):

});

export const ButtonStack = styled.div({

Nothing to fix now, but just a thought for later. We already have a component in AppButton.tsx called ButtonGroup. Maybe we could alignt these. That one shouldn't be needed when we have this one. But we don't need to replace all uses of that one now unless you want to.


desktop/packages/mullvad-vpn/src/renderer/components/Launch.tsx line 31 at r6 (raw file):

const StyledFooter = styled(Footer)({
  backgroundColor: colors.blue,
  padding: `0 14px ${spacings.spacing6}`,

Don't we want the vertical view margin here? And since it is extending Footer which already has padding bottom set to verticalViewMargin we shouldn't need to set it at all :)

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