Skip to content

feat(Dropdowns): Clockface 4 dropdown updates #876

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

Merged
ChitlangeSahas merged 5 commits into from
Oct 19, 2022
Merged

feat(Dropdowns): Clockface 4 dropdown updates #876

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
54bc89c
feat(Dropdowns): Clockface 4 updates for Dropdowns
ChitlangeSahas Oct 17, 2022
1049876
fix: hover effect
ChitlangeSahas Oct 18, 2022
01b5c45
fix: input border
ChitlangeSahas Oct 18, 2022
64c59bb
fix: input border focused
ChitlangeSahas Oct 18, 2022
0b4004a
fix: line margin
ChitlangeSahas Oct 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions src/Components/Button/Button.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,12 @@

.cf-button-primary,
.cf-button-success {
@include buttonColorModifier($cf-turquoise, $cf-turquoise, $cf-grey-1, $cf-grey-1);
@include buttonColorModifier(
$cf-turquoise,
$cf-turquoise,
$cf-grey-1,
$cf-grey-1
);
Comment on lines +225 to +230

@ChitlangeSahas ChitlangeSahas Oct 18, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Just ran prettier, hence the diff.

}

.cf-button-tertiary {
Expand All @@ -237,10 +242,15 @@

.cf-button-warning,
.cf-button-danger {
@include buttonColorModifier($cf-carnation, $cf-carnation, $cf-grey-1, $cf-grey-1);
@include buttonColorModifier(
$cf-carnation,
$cf-carnation,
$cf-grey-1,
$cf-grey-1
);

&:focus{
@include focus-shadow($cf-carnation)
&:focus {
@include focus-shadow($cf-carnation);
}
}

Expand Down
20 changes: 17 additions & 3 deletions src/Components/Dropdowns/Dropdown.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ $dropdown-item--padding-v: $cf-space-2xs;
border-radius: $cf-radius;
box-shadow: 0 2px 5px 0.6px rgba($g0-obsidian, 0.2);
padding: $cf-space-2xs;
border: rgba(255, 255, 255, 0.05) solid $cf-border;
}

.cf-dropdown-menu--contents {
Expand All @@ -52,6 +53,8 @@ $dropdown-item--padding-v: $cf-space-2xs;
flex-direction: column;
align-items: stretch;
cursor: pointer;
margin-right: 2px;
margin-left: 1px;
Comment on lines +56 to +57

@ChitlangeSahas ChitlangeSahas Oct 18, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

These two stick out, but they're just to please the absolute positioned, Scrollbar library we're using

@hoorayimhelping hoorayimhelping Oct 18, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can this be solved using position and left and right rather than with using margins? Semantically, there's a big difference between position and margin, and we should use positioning for laying things out, and margins for when we need space around an element. I'm not super familiar with how this is used, though

@ChitlangeSahas ChitlangeSahas Oct 18, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Got you! I'll try that!

@ChitlangeSahas ChitlangeSahas Oct 18, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Screen Shot 2022-10-18 at 8 19 39 AM

It still cuts off the right end of it, I think this is what is happening:

IMG_D4759C8952C6-1

So, when I add the margin, the border falls under the available space.

Unfortunately, I am not able to change the width of the inner wrapper, since it's controlled by the react-custom-scrollbar library.

@ChitlangeSahas ChitlangeSahas Oct 18, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This was always a problem, but we never noticed because we didn't use borders as an indicator for being selected. Now that we use the border, we see that the 1px of the right is cut off.

@taramk taramk Oct 19, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

is it causing problems to have the border? julia and i also discussed maybe not needing a background color at all on selected items. we included it because that's what we have now—adjusted to be lower contrast so it's less distracting—but i think the selected state for the radio buttons and checkboxes has enough contrast that we might not need the background color+border as well. let me know what you think. @ChitlangeSahas @hoorayimhelping @juliajames12

@ChitlangeSahas ChitlangeSahas Oct 19, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Currently I implemented it in the way figma looks, with no issues. But if we decide to remove the border+background, let me know I can make a future ticket and address that! 👍

@taramk taramk Oct 19, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

talked to sahas—it's a big of a bigger project to reevaluate these, so we'll keep it as-is for now and tweak it later if we find we need to once we test it out

}

/*
Expand All @@ -69,10 +72,18 @@ $dropdown-item--padding-v: $cf-space-2xs;
.cf-dropdown-divider {
@include dropdownItemStyles();
font-weight: $cf-font-weight--medium;
text-transform: uppercase;
color: #88889b;

@wdoconnell wdoconnell Oct 18, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since this is just a divider - does text-transform: uppercase have any effect?

@ChitlangeSahas ChitlangeSahas Oct 18, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Screen Shot 2022-10-18 at 9 17 47 AM

The dividers have text, optionally 👍

@wdoconnell wdoconnell Oct 18, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Gotcha - thanks!

padding-bottom: 4px;
border-bottom: $cf-grey-4 1px solid;
margin-bottom: 4px;

&.line {
padding: 0;
height: $cf-border;
background: $cf-grey-4;
margin: 8px 0 4px;
width: 100%;
}

&:hover {
Expand All @@ -85,6 +96,7 @@ $dropdown-item--padding-v: $cf-space-2xs;
color: $g20-white;
position: relative;
border-radius: $cf-radius;
margin-top: $cf-space-3xs;

&.cf-dropdown-link-item {
padding: 0;
Expand Down Expand Up @@ -287,12 +299,14 @@ $dropdown-item--padding-v: $cf-space-2xs;

.cf-dropdown-item:not(.cf-dropdown-item__disabled):hover,
.cf-dropdown-item:not(.cf-dropdown-item__disabled):focus {
background-color: rgba($cf-grey-95, 0.1);
background-color: rgba($cf-grey-95, 0.09);
outline: none;
}

.cf-dropdown-item.active:not(.cf-dropdown-item__disabled) {
background-color: $active;
background-color: $cf-martinique;
border: 1px $cf-lavender solid;
border-radius: $cf-border;
}
}

Expand All @@ -309,7 +323,7 @@ $dropdown-item--padding-v: $cf-space-2xs;
}

.cf-dropdown__onyx {
@include dropdownMenuColor($c-pool);
@include dropdownMenuColor($cf-lavender);
}

.cf-dropdown__none {
Expand Down
2 changes: 0 additions & 2 deletions src/Components/Dropdowns/DropdownMenu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,6 @@ export const DropdownMenu = forwardRef<DropdownMenuRef, DropdownMenuProps>(
[`cf-dropdown__${theme}`]: theme,
})

// const {thumbStartColor, thumbStopColor} = getScrollbarColorsFromTheme(theme)

const scrollTop = calculateSelectedPosition(scrollToSelected, children)

const scrollbarsStyle = {
Expand Down
1 change: 1 addition & 0 deletions src/Components/Inputs/Input.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ alternate css for the other sizes */
&:before,
&:after {
width: $cf-form-sm-font;
background: $cf-input-text--default;
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/Styles/variables.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,8 +330,8 @@ $cf-input-text--focused: $cf-white;
$cf-input-text--hover: $cf-white;
$cf-input-text--disabled: $cf-empty-state-text;

$cf-input-border--default: $g5-pepper;
$cf-input-border--focused: rgba($c-pool, 0.75);
$cf-input-border--default: $cf-grey-4;
$cf-input-border--focused: rgba($cf-turquoise, 0.75);
$cf-input-border--hover: $g5-pepper;
$cf-input-border--disabled: $g4-onyx;

Expand Down