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

#17 Add Save Layout UI Component #29

Conversation

pling-scottlogic
Copy link

Description

Adds a UI component to handle saving custom layouts. Closes #17.

Change List

  • Add SaveLayoutPanel component
  • Add Showcase example for SaveLayoutPanel
  • Add sub-components for checkboxes, radio buttons (and icons) and chevron icons
  • Add radio and chevron SVG icons to index.css
  • Add chevron icon to ComboBox
  • Adjust automatic focus and make close button optional in Dialog component
  • Update the "New Theme" example to log details after clicking save

Screenshots

image

image

Testing

Manual testing covered keyboard as well as mouse navigation. All user-provided inputs - layout name, group, checkbox and radio values - are logged to the console correctly (see second screenshot).

Notes

CI checks are not expected to pass until #23 is merged into the base branch.

<Button className="cancelButton" onClick={onCancel}>
Cancel
</Button>
<Button className="saveButton" onClick={() => onSave(layoutName, group, checkValues, radioValue || "")}

Choose a reason for hiding this comment

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

I think this would look cleaner if every prop was on a new line

background: var(--actionable-primary-background-default, #FFF);
}

.cancelButton.saveButton.saltButton:hover {

Choose a reason for hiding this comment

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

I think this can be removed as it is unused. this styling will be applied to elements with className "cancelButton saveButton saltButton" and our buttons have either "cancelButton" or "saveButton" as classNames but not both. I think the intention was to do something like on line 82 (to style both classes use ",")

- Temporary change to fix build until screenshot functionality is implemented
Copy link

@cfisher-scottlogic cfisher-scottlogic left a comment

Choose a reason for hiding this comment

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

I've made a few comments/questions for the code.

I also have some UI-related comments comparing the actual UI to the Figma designs:
image

  1. It looks like the font is a different colour
  2. The selected radio button inner white circle is a different size
  3. There's less (left) padding in the dropdown boxes (are these called combo boxes?)

None of it is major stuff though, so nice work 🙂

vuu-ui/packages/vuu-icons/index.css Outdated Show resolved Hide resolved
vuu-ui/packages/vuu-popups/src/dialog/Dialog.tsx Outdated Show resolved Hide resolved
vuu-ui/packages/vuu-ui-controls/src/combo-box/ComboBox.tsx Outdated Show resolved Hide resolved
vuu-ui/packages/vuu-ui-controls/src/inputs/Checkbox.tsx Outdated Show resolved Hide resolved
vuu-ui/packages/vuu-ui-controls/src/combo-box/ComboBox.tsx Outdated Show resolved Hide resolved
vuu-ui/packages/vuu-popups/src/dialog/Dialog.tsx Outdated Show resolved Hide resolved
vuu-ui/packages/vuu-ui-controls/src/list/RadioIcon.tsx Outdated Show resolved Hide resolved
background: var(--actionable-primary-background-default, #FFF);
}

.saveButton {
Copy link

Choose a reason for hiding this comment

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

class names like this maybe a bit too generic, I tend to prefix them with component name, although they likely won't need individual styling like this once all the theme styles are in place.

Copy link

@cfisher-scottlogic cfisher-scottlogic left a comment

Choose a reason for hiding this comment

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

LGTM 🙂

@pling-scottlogic
Copy link
Author

I've made a few comments/questions for the code.

I also have some UI-related comments comparing the actual UI to the Figma designs: image

1. It looks like the font is a different colour

2. The selected radio button inner white circle is a different size

3. There's less (left) padding in the dropdown boxes (are these called combo boxes?)

None of it is major stuff though, so nice work 🙂

  1. I'm seeing the same hex value in Figma and dev tools, so I wonder if there's a screenshot compression issue here? There are different fonts on the Figma design though, so we probably want to ask Justyna about that.
  2. Fixed in 7af9ecc
  3. The styling on these inputs is coming from the Input component in Salt Core. If we want this to match the design perfectly, we should probably build our own custom Input component (based on the Salt one); but as it's such a minor difference, maybe we can just seek design sign-off on it.

@pling-scottlogic pling-scottlogic merged commit 7f66f86 into feature/layout-management/layout-management-feature Sep 1, 2023
6 checks passed
@pling-scottlogic pling-scottlogic deleted the feature/VUU-17-layoutSave branch September 1, 2023 08:39
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.

Create Save Layout dialog component
4 participants