Update styling for empty button and date picker icon #1342

ps48 · 2024-08-13T18:05:04Z

Description

Update styling for empty button and date picker icon alignments issues:

to the left of the search bar is the save icon. it feels 1px lower than middle and 1px left of center.

Issues Resolved

Enables button icon to be in right line-height based.
Reduces the spacing in between calendar button and pop over icon in super date picker

Before:

After:

Check List

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Shenoy Pratik <[email protected]>

virajsanghvi

just some early feedback

virajsanghvi · 2024-08-13T21:01:39Z

src/components/date_picker/super_date_picker/quick_select_popover/_quick_select_popover.scss

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


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

Also, how did you determine this size?

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

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.

virajsanghvi · 2024-08-13T21:03:12Z

src/components/button/button_empty/_button_empty.scss

@@ -96,6 +96,7 @@ $ouiButtonEmptyTypes: (
      // Ghost is unique and ALWAYS sits against a dark background.
      color: $color;
    } @else if ($name == 'text') {
+      line-height: inherit;


Is the goal for empty buttons to be aligned with other text? How does this impact alignment with other buttons?

Makes sense, updated to parent class ouiButtonEmpty. Also, this only impacts ouiButtonEmpty which brings this component inline with other button icons:

Signed-off-by: Shenoy Pratik <[email protected]>

AMoo-Miki · 2024-08-14T22:39:39Z

src/components/date_picker/super_date_picker/quick_select_popover/_quick_select_popover.scss

-.ouiQuickSelectPopover__buttonText {
-  // Override specificity from universal and sibling selectors
-  margin-right: $ouiSizeXS !important;


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?

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

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.

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

It is expected users can use anything/everything exposed by OUI from components to styling.

We shouldn’t break existing stuff for plugins/core or community partners

virajsanghvi · 2024-08-14T22:56:56Z

src/components/date_picker/super_date_picker/quick_select_popover/_quick_select_popover.scss

-.ouiQuickSelectPopover__buttonText {
-  // Override specificity from universal and sibling selectors
-  margin-right: $ouiSizeXS !important;
+.ouiFormControlLayout__prepend{


nit: space before {?

removed the prepend class based on Miki's comment

virajsanghvi · 2024-08-14T22:58:37Z

src/components/date_picker/super_date_picker/quick_select_popover/_quick_select_popover.scss

-.ouiQuickSelectPopover__buttonText {
-  // Override specificity from universal and sibling selectors
-  margin-right: $ouiSizeXS !important;


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.

virajsanghvi · 2024-08-14T23:01:33Z

src/components/date_picker/super_date_picker/quick_select_popover/_quick_select_popover.scss

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


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.

Signed-off-by: Shenoy Pratik <[email protected]>

ps48 · 2024-08-15T16:15:16Z

@virajsanghvi Without the calc function I see deprecation warning from Sass:

Deprecation Warning: Using / for division outside of calc() is deprecated and will be removed in Dart Sass 2.0.0.

Recommendation: math.div($ouiSizeXS, 2) or calc($ouiSizeXS / 2)

More info and automated migrator: https://sass-lang.com/d/slash-div

   ╷
28 │   margin-right: $ouiSizeXS/2 !important;
   │                 ^^^^^^^^^^^^
   ╵

Signed-off-by: Shenoy Pratik <[email protected]>

virajsanghvi

Approving assuming it passes checks

* update styling for empty button and date picker icon Signed-off-by: Shenoy Pratik <[email protected]> * move line-height to empty button class Signed-off-by: Shenoy Pratik <[email protected]> * update changelog Signed-off-by: Shenoy Pratik <[email protected]> * remove specificity for QuickSelectPopover Signed-off-by: Shenoy Pratik <[email protected]> * fix formatting in for calc Signed-off-by: Shenoy Pratik <[email protected]> --------- Signed-off-by: Shenoy Pratik <[email protected]> (cherry picked from commit d2bea6f) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md

* update styling for empty button and date picker icon Signed-off-by: Shenoy Pratik <[email protected]> * move line-height to empty button class Signed-off-by: Shenoy Pratik <[email protected]> * update changelog Signed-off-by: Shenoy Pratik <[email protected]> * remove specificity for QuickSelectPopover Signed-off-by: Shenoy Pratik <[email protected]> * fix formatting in for calc Signed-off-by: Shenoy Pratik <[email protected]> --------- Signed-off-by: Shenoy Pratik <[email protected]> (cherry picked from commit d2bea6f) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

update styling for empty button and date picker icon

d51c6ae

Signed-off-by: Shenoy Pratik <[email protected]>

github-actions bot added the first-time-contributor label Aug 13, 2024

virajsanghvi reviewed Aug 13, 2024

View reviewed changes

ps48 added 2 commits August 14, 2024 10:18

move line-height to empty button class

a95e4d2

Signed-off-by: Shenoy Pratik <[email protected]>

update changelog

331261c

Signed-off-by: Shenoy Pratik <[email protected]>

ps48 marked this pull request as ready for review August 14, 2024 18:34

ps48 requested review from AMoo-Miki, rednaksi91, bandinib-amzn, ashwin-pc, joshuarrrr, andreymyssak and SergeyMyssak as code owners August 14, 2024 18:34

AMoo-Miki reviewed Aug 14, 2024

View reviewed changes

virajsanghvi reviewed Aug 14, 2024

View reviewed changes

remove specificity for QuickSelectPopover

555b623

Signed-off-by: Shenoy Pratik <[email protected]>

fix formatting in for calc

59cc727

Signed-off-by: Shenoy Pratik <[email protected]>

virajsanghvi approved these changes Aug 15, 2024

View reviewed changes

AMoo-Miki added the backport 1.x label Aug 16, 2024

AMoo-Miki merged commit d2bea6f into opensearch-project:main Aug 16, 2024
16 checks passed

opensearch-trigger-bot bot mentioned this pull request Aug 16, 2024

[Backport 1.x] Update styling for empty button and date picker icon #1348

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update styling for empty button and date picker icon #1342

Update styling for empty button and date picker icon #1342

ps48 commented Aug 13, 2024 •

edited

Loading

virajsanghvi left a comment

virajsanghvi Aug 13, 2024

ps48 Aug 14, 2024

virajsanghvi Aug 14, 2024

virajsanghvi Aug 13, 2024

ps48 Aug 14, 2024

AMoo-Miki Aug 14, 2024

ps48 Aug 14, 2024

virajsanghvi Aug 14, 2024 •

edited

Loading

ps48 Aug 15, 2024

virajsanghvi Aug 14, 2024

ps48 Aug 15, 2024

virajsanghvi Aug 14, 2024 •

edited

Loading

virajsanghvi Aug 14, 2024

ps48 commented Aug 15, 2024

virajsanghvi left a comment

Update styling for empty button and date picker icon #1342

Update styling for empty button and date picker icon #1342

Conversation

ps48 commented Aug 13, 2024 • edited Loading

Description

Issues Resolved

Check List

virajsanghvi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

virajsanghvi Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

virajsanghvi Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ps48 commented Aug 15, 2024

virajsanghvi left a comment

Choose a reason for hiding this comment

ps48 commented Aug 13, 2024 •

edited

Loading

virajsanghvi Aug 14, 2024 •

edited

Loading

virajsanghvi Aug 14, 2024 •

edited

Loading