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

#5575 Configuration options styling #5617

Merged
merged 7 commits into from Apr 27, 2023

Conversation

turbochef
Copy link
Contributor

@turbochef turbochef commented Apr 26, 2023

What does this PR do?

Demo

Future Work

  • Remove the “Input” title on the initial options
  • Update all instances of primary buttons in the configuration settings to borderless/ghost

Checklist

  • Designate a primary reviewer

@turbochef turbochef requested a review from BLoe April 26, 2023 02:44
@twschiller twschiller added this to the 1.7.25 milestone Apr 27, 2023
@twschiller
Copy link
Contributor

Added @mnholtz and @BrandonPxBx as FYI. PR/code changes are still in flux currently

@codecov
Copy link

codecov bot commented Apr 27, 2023

Codecov Report

Merging #5617 (b788409) into main (764bf25) will decrease coverage by 0.11%.
The diff coverage is 57.62%.

@@            Coverage Diff             @@
##             main    #5617      +/-   ##
==========================================
- Coverage   64.43%   64.32%   -0.11%     
==========================================
  Files        1031     1031              
  Lines       32438    32414      -24     
  Branches     6155     6155              
==========================================
- Hits        20900    20850      -50     
- Misses      11538    11564      +26     
Impacted Files Coverage Δ
...ents/selectionToolPopover/SelectionToolPopover.tsx 42.50% <0.00%> (-1.41%) ⬇️
...c/pageEditor/tabs/sidebar/SidebarConfiguration.tsx 38.70% <0.00%> (-3.72%) ⬇️
src/pageEditor/tabs/tour/TourConfiguration.tsx 90.00% <0.00%> (-1.67%) ⬇️
...quickBarProvider/QuickBarProviderConfiguration.tsx 54.54% <12.50%> (-1.98%) ⬇️
...itor/tabs/contextMenu/ContextMenuConfiguration.tsx 92.85% <50.00%> (-0.48%) ⬇️
...c/pageEditor/tabs/trigger/TriggerConfiguration.tsx 56.86% <66.66%> (-1.63%) ⬇️
src/pageEditor/tabs/effect/BlockConfiguration.tsx 93.44% <71.42%> (-0.21%) ⬇️
src/pageEditor/tabs/editTab/EditTab.tsx 97.77% <80.00%> (ø)
...onents/fields/schemaFields/widgets/ArrayWidget.tsx 86.84% <100.00%> (+0.35%) ⬆️
src/pageEditor/fields/AccordionFieldSection.tsx 100.00% <100.00%> (ø)
... and 5 more

... and 1 file with indirect coverage changes

@@ -42,13 +42,18 @@ $border: rgba(0, 0, 0, 0.3);
display: flex;
align-items: center;
cursor: move;

padding: 12px;
background: #ddd;
Copy link
Collaborator

@mnholtz mnholtz Apr 27, 2023

Choose a reason for hiding this comment

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

nit: Would you add this color to colors.scss and import it as a variable, e.g.

@import "@/themes/colors";
?

You can give it an arbitrary name that makes sense because it's not in our Design System. We are attempting to start collecting hard-coded values as css variables as part of a first step in improving the state of our design system.

Copy link
Collaborator

Choose a reason for hiding this comment

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

See this PR for context: #4726

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood. I'll address both.

padding: 8px;
border-radius: 4px;
font-weight: 500;
background: #f8f9fa;
Copy link
Collaborator

@mnholtz mnholtz Apr 27, 2023

Choose a reason for hiding this comment

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

nit: see other comment re refactoring this into a color in colors.scss

Copy link
Collaborator

@BLoe BLoe left a comment

Choose a reason for hiding this comment

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

Just had a small question about the CSS class that you added, LGTM otherwise 👍

"matching elements"
)}`}
</div>
<div className="d-flex align-items-center popover-wrapper-body">
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the effect of adding popover-wrapper-body here? Was this moved from somewhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just styling. FieldSection was providing some padding but since I replaced it with a div I'm using this for padding.

@github-actions
Copy link

When the PR is merged, the first loom link found on this PR will be posted to #sprint-demo on Slack. Do not edit this comment manually.

@turbochef turbochef merged commit 81b178e into main Apr 27, 2023
17 checks passed
@turbochef turbochef deleted the feature/5576-configuration-accordion branch April 27, 2023 18:56
@twschiller twschiller modified the milestones: 1.7.25, 1.7.26, 1.7.27 Apr 28, 2023
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.

Page Editor Epic Slice 3a: "Unbox" the "Configuration"
4 participants