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.

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

Update styling for empty button and date picker icon #1342

Merged
merged 5 commits into from
Aug 16, 2024
Merged

Update styling for empty button and date picker icon #1342

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d51c6ae
update styling for empty button and date picker icon
ps48 Aug 13, 2024
a95e4d2
move line-height to empty button class
ps48 Aug 14, 2024
331261c
update changelog
ps48 Aug 14, 2024
555b623
remove specificity for QuickSelectPopover
ps48 Aug 14, 2024
59cc727
fix formatting in for calc
ps48 Aug 15, 2024
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
### 🐛 Bug Fixes

- Fixes combobox height with appendees ([#1338](https://github.com/opensearch-project/oui/pull/1338))
- Update styling for empty button and date picker icon ([#1342](https://github.com/opensearch-project/oui/pull/1342))

### 🚞 Infrastructure

Expand Down
1 change: 1 addition & 0 deletions src/components/button/button_empty/_button_empty.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
animation: none !important; /* 1 */
transition-timing-function: ease-in; /* 2 */
transition-duration: $ouiAnimSpeedFast; /* 2 */
line-height: inherit;

.ouiButtonEmpty__content {
padding: 0 $ouiSizeS;
Expand Down
8 changes: 5 additions & 3 deletions src/components/date_picker/super_date_picker/quick_select_popover/_quick_select_popover.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,11 @@
}

// sass-lint:disable no-important
.ouiQuickSelectPopover__buttonText {
// Override specificity from universal and sibling selectors
margin-right: $ouiSizeXS !important;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see that the new rule applies to elements with classname ouiQuickSelectPopover__buttonText when inside a prepend but it removes the rule that applied margin to all other elements with classname ouiQuickSelectPopover__buttonText. Shouldn't we retain that? Are we not breaking anything else by removing this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did check it, every repo in the opensearch-project is using this class for date-picker. It is only dashboards which is using this class for the save button.

Class references: https://github.com/search?q=org%3Aopensearch-project+euiQuickSelectPopover__buttonText&type=code

Copy link
Collaborator

@virajsanghvi virajsanghvi Aug 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

is this class only used by explicit classname or is it applied by a component? If by component, I'm not sure if your search is sufficient for usage.

Copy link
Member Author

Choose a reason for hiding this comment

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

Reverted to original class. Reasoning based on offline discussion with @AMoo-Miki since, OUI is a styling library:

  1. It is expected users can use anything/everything exposed by OUI from components to styling.
  2. We shouldn’t break existing stuff for plugins/core or community partners

.ouiFormControlLayout__prepend{
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: space before {?

Copy link
Member Author

Choose a reason for hiding this comment

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

removed the prepend class based on Miki's comment

.ouiQuickSelectPopover__buttonText {
// Override specificity from universal and sibling selectors
margin-right: calc($ouiSizeXS/2) !important;
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do you need calc when this will resolve to a value?

Also, how did you determine this size?

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently, this is how it is done in rest of OUI rather than using constants. This approach may be helpful in making things extensible when a theme comes in and changes the $ouiSizeXS from 4px to 8px. Other places using this:
https://github.com/search?q=repo%3Aopensearch-project%2Foui+calc%28%24ouiSizeXS+%2F+2%29&type=code

Please let me know if this isn't recommended can hard code to margin-right: 2px

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, my point is that calc is a css function, but I believe, maybe wrong, that the expression $ouiSize/2 will be evaluated by sass first, so the resulting css will be margin-right: calc(2px) !important - basically my point is I don't think calc is doing anything here, and not sure how it helps make it extensible unless we had css variables.

}
}

.ouiQuickSelectPopover__anchor {
Expand Down
Loading