feat: add self-contained styles for Time component #269

markpalfreeman · 2019-08-16T17:53:17Z

Related Issue

Description

Self-contained styles for Time.

NOTE / HELP WANTED:
I removed the selector at this level that shrinks its child input padding for these small inputs.
I need guidance on where this should go instead—I don't think we want that for all .fd-form__controls, it's specific to this context 🤷‍♂ addressed

Screenshots

Before:

After:

~~❗️ notice the padding issue now that it's been reverted the "default"~~ addressed

netlify · 2019-08-16T17:56:50Z

Deploy preview for fundamental-styles ready!

Built with commit 80fd778

https://deploy-preview-269--fundamental-styles.netlify.com

markpalfreeman · 2019-08-16T17:54:05Z

scss/components/time.scss

      &:last-child {
        margin-right: 0;
      }
    }

    &__input {
-        margin: fd-space(2) 0;
-        input {


Most primary change—the removal of this padding messes things up (reverts to a larger side padding), but I'm not sure where to put it instead.

🤔 You are right that these styles are specific to this context. We have n-number of "items", each with an input and two buttons. Let's style it that way. Let's keep the fd-time__item styles. Then, let's remove (🔥) the current fd-time__input wrapper <div> and put that class directly on the <input> and make the styling:

&__input { margin: fd-space(2) 0; padding-left: fd-space(2); padding-right: fd-space(2); text-align: center; }

I would also put the fd-time__input as a child selector to fd-time__item to make sure our specificity wins on the <input> (since that will have styles already coming from fd-form__control).

This will all be a breaking change, but hey -- we're on version 0. 😄

This makes a lot of sense, good suggestion on just repurposing the class (maybe that's what Jenna was hinting at earlier and I didn't fully grasp it).

markpalfreeman · 2019-08-16T17:55:28Z

test/resources/layout-shared.css

@@ -25,7 +25,8 @@ body {
 }

 .input-playground {
-  width: 250px;
+  width: 100%;


This is the default input style, which I think makes sense to have here too; without it, playground inputs with "limited" width extend way beyond their containers.

markpalfreeman · 2019-08-16T17:57:13Z

test/templates/time/component.njk

  {%- for item in properties.items %}
    {{ time_item(item) }}
  {%- endfor %}
 </div>
 {%- endmacro %}

-{%- macro time_item(properties={}, state={}, modifier=[], classes=[], aria=[]) -%}
+{%- macro time_item(properties={}) -%}


Cleaning up properties that aren't needed with just a single example.

greg-a-smith · 2019-08-16T21:37:57Z

scss/components/time.scss

      &:last-child {
        margin-right: 0;
      }
    }

    &__input {
-        margin: fd-space(2) 0;
-        input {


🤔 You are right that these styles are specific to this context. We have n-number of "items", each with an input and two buttons. Let's style it that way. Let's keep the fd-time__item styles. Then, let's remove (🔥) the current fd-time__input wrapper <div> and put that class directly on the <input> and make the styling:

&__input { margin: fd-space(2) 0; padding-left: fd-space(2); padding-right: fd-space(2); text-align: center; }

I would also put the fd-time__input as a child selector to fd-time__item to make sure our specificity wins on the <input> (since that will have styles already coming from fd-form__control).

This will all be a breaking change, but hey -- we're on version 0. 😄

BREAKING CHANGE

markpalfreeman · 2019-08-16T22:19:06Z

Fixes addressed, the docs page has been restored:

greg-a-smith · 2019-08-17T03:02:26Z

components/time.scss

@@ -0,0 +1,4 @@
+$fd-fonts-path: "../scss/fonts/";
+
+@import "../scss/fonts/72";


Not sure why I didn't see this before, but the font isn't actually needed for this component. The form input will bring the font with it.

greg-a-smith · 2019-08-17T03:15:17Z

scss/components/time.scss

+      margin: fd-space(2) 0;
+      padding-left: fd-space(2);
+      padding-right: fd-space(2);
+      text-align: center;


You can leverage the power of sass and write this like:

&__item { {{ existing styles }} .#{$block}__input { margin: fd-space(2) 0; padding-left: fd-space(2); padding-right: fd-space(2); text-align: center; } }

I think it's safer to write fd-time__input as a descendant class to fd-time__item, but not necessarily a direct descendant (using the > notation).

Yeah I went back and forth on both of those points :)

Changes made.

greg-a-smith · 2019-08-19T14:53:14Z

scss/components/time.scss

@@ -11,17 +11,17 @@ $block: #{$fd-namespace}-time;
      max-width: fd-space(10);
      text-align: center;
      margin-right: fd-space(1);
+


I think I missed this before (probably because it didn't show up directly in the file diff), but what exactly is this width: 184px for? The phrase "quick fix" worries me. And what overflow issue? This seems like it might be related to the use of fd-time within fd-time-picker, but this strikes me as a problem. In fact, see the attached screenshot.

The 4th __item is wrapping due to this arbitrary width. I know it's the playground with crazy margin values being applied, but there is still no reason why any of this should wrap to a new line.

Maybe the fd-time class should provide a flex box wrapper around the fd-time__item blocks to prevent them from ever "wrapping". Thoughts?

I agree that it should be changed (any fixed width seems like a hack), but I wasn't sure this was the right time to do it...

You know what, maybe it isn't the time to do it. I've created #280 to track this work.

greg-a-smith

With the hard-coded width issue being pushed to a separate issue, this looks good. 🚢

markpalfreeman · 2019-08-20T16:00:40Z

@greg-a-smith thanks, I agree. I'm wrapping up some TimePicker changes I was going to bundle here, but for the sake of reviews I'll just open another small PR

fix: add self-contained styles for Time component

f8a4233

markpalfreeman added Help wanted Extra attention is needed Ready for Review labels Aug 16, 2019

markpalfreeman requested a review from a team August 16, 2019 17:53

markpalfreeman self-assigned this Aug 16, 2019

markpalfreeman commented Aug 16, 2019

View reviewed changes

markpalfreeman added the self-contained label Aug 16, 2019

greg-a-smith suggested changes Aug 16, 2019

View reviewed changes

fix: use time input class for margin/padding

33d74e6

BREAKING CHANGE

markpalfreeman requested a review from greg-a-smith August 16, 2019 22:19

markpalfreeman removed the Help wanted Extra attention is needed label Aug 16, 2019

greg-a-smith suggested changes Aug 17, 2019

View reviewed changes

fix: clean up Time scss

80fd778

greg-a-smith suggested changes Aug 19, 2019

View reviewed changes

greg-a-smith approved these changes Aug 20, 2019

View reviewed changes

greg-a-smith changed the title ~~fix: add self-contained styles for Time component~~ feat: add self-contained styles for Time component Aug 20, 2019

markpalfreeman merged commit 54b7b1e into SAP:master Aug 20, 2019

jbadan mentioned this pull request Aug 20, 2019

fix: revert multi input test/new shared layout input class #284

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add self-contained styles for Time component #269

feat: add self-contained styles for Time component #269

markpalfreeman commented Aug 16, 2019 •

edited

Loading

netlify bot commented Aug 16, 2019 •

edited

Loading

markpalfreeman Aug 16, 2019 •

edited

Loading

greg-a-smith Aug 16, 2019

markpalfreeman Aug 16, 2019

markpalfreeman Aug 16, 2019

markpalfreeman Aug 16, 2019

greg-a-smith Aug 16, 2019

markpalfreeman commented Aug 16, 2019

greg-a-smith Aug 17, 2019

greg-a-smith Aug 17, 2019

markpalfreeman Aug 19, 2019

greg-a-smith Aug 19, 2019

markpalfreeman Aug 19, 2019

greg-a-smith Aug 20, 2019

greg-a-smith left a comment

markpalfreeman commented Aug 20, 2019

		@@ -0,0 +1,4 @@
		$fd-fonts-path: "../scss/fonts/";

		@import "../scss/fonts/72";

feat: add self-contained styles for Time component #269

feat: add self-contained styles for Time component #269

Conversation

markpalfreeman commented Aug 16, 2019 • edited Loading

Related Issue

Description

Screenshots

Before:

After:

netlify bot commented Aug 16, 2019 • edited Loading

markpalfreeman Aug 16, 2019 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markpalfreeman commented Aug 16, 2019

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

greg-a-smith left a comment

Choose a reason for hiding this comment

markpalfreeman commented Aug 20, 2019

markpalfreeman commented Aug 16, 2019 •

edited

Loading

netlify bot commented Aug 16, 2019 •

edited

Loading

markpalfreeman Aug 16, 2019 •

edited

Loading