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

Jump to bottom

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

Merged
merged 3 commits into from
Aug 20, 2019
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f8a4233
fix: add self-contained styles for Time component
Aug 16, 2019
33d74e6
fix: use time input class for margin/padding
Aug 16, 2019
80fd778
fix: clean up Time scss
Aug 19, 2019
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 components/time.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
@import "../scss/components/time";
50 changes: 10 additions & 40 deletions docs/pages/components/time.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,7 @@ Multiple instances can be used in the `time-picker` to choose hours, minutes, se
<button class="fd-button--light fd-button--xs sap-icon--navigation-up-arrow"
aria-label="Increase hours" aria-controls="1610C873"></button>
</div>
<div class="fd-time__input">
<input class="fd-form__control" type="text" placeholder="hh" value="02"
id="1610C873" aria-label="Hours"/>
</div>
<input class="fd-time__input fd-form__control" type="text" placeholder="hh" value="02" id="1610C873" aria-label="Hours"/>
<div class="fd-time__control">
<button class="fd-button--light fd-button--xs sap-icon--navigation-down-arrow"
aria-label="Decrease hours" aria-controls="1610C873"></button>
Expand All @@ -40,10 +37,7 @@ Multiple instances can be used in the `time-picker` to choose hours, minutes, se
<button class="fd-button--light fd-button--xs sap-icon--navigation-up-arrow"
aria-label="Increase minutes" aria-controls="DDlHR199"></button>
</div>
<div class="fd-time__input">
<input class="fd-form__control" type="text" placeholder="mm" value="34" id="DDlHR199"
aria-label="Minutes"/>
</div>
<input class="fd-time__input fd-form__control" type="text" placeholder="mm" value="34" id="DDlHR199" aria-label="Minutes"/>
<div class="fd-time__control">
<button class="fd-button--light fd-button--xs sap-icon--navigation-down-arrow"
aria-label="Decrease minutes" aria-controls="DDlHR199"></button>
Expand All @@ -54,10 +48,7 @@ Multiple instances can be used in the `time-picker` to choose hours, minutes, se
<button class="fd-button--light fd-button--xs sap-icon--navigation-up-arrow"
aria-label="Increase seconds" aria-controls="8CAnL947"></button>
</div>
<div class="fd-time__input">
<input class="fd-form__control" type="text" placeholder="ss" value="56" id="8CAnL947"
aria-label="Seconds"/>
</div>
<input class="fd-time__input fd-form__control" type="text" placeholder="ss" value="56" id="8CAnL947" aria-label="Seconds"/>
<div class="fd-time__control">
<button class="fd-button--light fd-button--xs sap-icon--navigation-down-arrow"
aria-label="Decrease seconds" aria-controls="8CAnL947"></button>
Expand All @@ -68,10 +59,7 @@ Multiple instances can be used in the `time-picker` to choose hours, minutes, se
<button class="fd-button--light fd-button--xs sap-icon--navigation-up-arrow"
aria-label="Increase period" aria-controls="sEWOL676"></button>
</div>
<div class="fd-time__input">
<input class="fd-form__control" type="text" placeholder="am" value="pm" id="sEWOL676"
aria-label="Period"/>
</div>
<input class="fd-time__input fd-form__control" type="text" placeholder="am" value="pm" id="sEWOL676" aria-label="Period"/>
<div class="fd-time__control">
<button class="fd-button--light fd-button--xs sap-icon--navigation-down-arrow"
aria-label="Decrease period" aria-controls="sEWOL676"></button>
Expand All @@ -91,10 +79,7 @@ Multiple instances can be used in the `time-picker` to choose hours, minutes, se
<button class="fd-button--light fd-button--xs sap-icon--navigation-up-arrow"
aria-label="Increase hours" aria-controls="HgDLk176"></button>
</div>
<div class="fd-time__input">
<input class="fd-form__control" type="text" placeholder="hh" value="" id="HgDLk176"
aria-label="Hours"/>
</div>
<input class="fd-time__input fd-form__control" type="text" placeholder="hh" value="" id="HgDLk176" aria-label="Hours"/>
<div class="fd-time__control">
<button class="fd-button--light fd-button--xs sap-icon--navigation-down-arrow"
aria-label="Decrease hours" aria-controls="HgDLk176"></button>
Expand All @@ -105,10 +90,7 @@ Multiple instances can be used in the `time-picker` to choose hours, minutes, se
<button class="fd-button--light fd-button--xs sap-icon--navigation-up-arrow"
aria-label="Increase minutes" aria-controls="CHeFH472"></button>
</div>
<div class="fd-time__input">
<input class="fd-form__control" type="text" placeholder="mm" value="" id="CHeFH472"
aria-label="Minutes"/>
</div>
<input class="fd-time__input fd-form__control" type="text" placeholder="mm" value="" id="CHeFH472" aria-label="Minutes"/>
<div class="fd-time__control">
<button class="fd-button--light fd-button--xs sap-icon--navigation-down-arrow"
aria-label="Decrease minutes" aria-controls="CHeFH472"></button>
Expand All @@ -119,10 +101,7 @@ Multiple instances can be used in the `time-picker` to choose hours, minutes, se
<button class="fd-button--light fd-button--xs sap-icon--navigation-up-arrow"
aria-label="Increase seconds" aria-controls="qMPpb855"></button>
</div>
<div class="fd-time__input">
<input class="fd-form__control" type="text" placeholder="ss" value="" id="qMPpb855"
aria-label="Seconds"/>
</div>
<input class="fd-time__input fd-form__control" type="text" placeholder="ss" value="" id="qMPpb855" aria-label="Seconds"/>
<div class="fd-time__control">
<button class="fd-button--light fd-button--xs sap-icon--navigation-down-arrow"
aria-label="Decrease seconds" aria-controls="qMPpb855"></button>
Expand All @@ -133,10 +112,7 @@ Multiple instances can be used in the `time-picker` to choose hours, minutes, se
<button class="fd-button--light fd-button--xs sap-icon--navigation-up-arrow"
aria-label="Increase period" aria-controls="VpUG6928"></button>
</div>
<div class="fd-time__input">
<input class="fd-form__control" type="text" placeholder="am" value="" id="VpUG6928"
aria-label="Period"/>
</div>
<input class="fd-time__input fd-form__control" type="text" placeholder="am" value="" id="VpUG6928" aria-label="Period"/>
<div class="fd-time__control">
<button class="fd-button--light fd-button--xs sap-icon--navigation-down-arrow"
aria-label="Decrease period" aria-controls="VpUG6928"></button>
Expand All @@ -159,10 +135,7 @@ disabled when the first or last values are reached.
<button class="fd-button--light fd-button--xs sap-icon--navigation-up-arrow"
aria-label="Increase hours" aria-controls="Rjap5115"></button>
</div>
<div class="fd-time__input">
<input class="fd-form__control" type="text" placeholder="hh" value="00" id="Rjap5115"
aria-label="Hours"/>
</div>
<input class="fd-time__input fd-form__control" type="text" placeholder="hh" value="00" id="Rjap5115" aria-label="Hours"/>
<div class="fd-time__control">
<button class="fd-button--light fd-button--xs sap-icon--navigation-down-arrow is-disabled"
aria-label="Decrease hours" aria-controls="Rjap5115"></button>
Expand All @@ -173,10 +146,7 @@ disabled when the first or last values are reached.
<button class="fd-button--light fd-button--xs sap-icon--navigation-up-arrow is-disabled"
aria-label="Increase minutes" aria-controls="VnVPz732"></button>
</div>
<div class="fd-time__input">
<input class="fd-form__control" type="text" placeholder="mm" value="59" id="VnVPz732"
aria-label="Minutes"/>
</div>
<input class="fd-time__input fd-form__control" type="text" placeholder="mm" value="59" id="VnVPz732" aria-label="Minutes"/>
<div class="fd-time__control">
<button class="fd-button--light fd-button--xs sap-icon--navigation-down-arrow"
aria-label="Decrease minutes" aria-controls="VnVPz732"></button>
Expand Down
14 changes: 7 additions & 7 deletions scss/components/time.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,17 @@ $block: #{$fd-namespace}-time;
max-width: fd-space(10);
text-align: center;
margin-right: fd-space(1);

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Screen Shot 2019-08-19 at 9 49 30 AM

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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...

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

&__input {
// Spacing and narrower side padding for child inputs
.#{$block}__input {
margin: fd-space(2) 0;
input {
Copy link
Contributor Author

@markpalfreeman markpalfreeman Aug 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 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. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

padding-left: fd-space(2);
padding-right: fd-space(2);
text-align: center;
}
padding-left: fd-space(2);
padding-right: fd-space(2);
text-align: center;
}
}
}
3 changes: 2 additions & 1 deletion test/resources/layout-shared.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ body {
}

.input-playground {
width: 250px;
width: 100%;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

box-sizing: border-box;
background: beige;
padding: 10px;
font-size: 14px;
Expand Down
33 changes: 7 additions & 26 deletions test/templates/time/component.njk
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,48 +1,29 @@
{% import "./../utils.njk" as utils %}
{% from "../button/component.njk" import button %}

<!--
menu:
time:
properties={},
modifier={ block: [] },
state={},
aria={}
-->


{%- macro time(properties={}, state={}, modifier=[], classes=[], aria=[]) -%}
<div class="fd-time{{ modifier | modifier('time') }}{{ classes | classes }}"{{ aria | aria }}>
{%- macro time(properties={}, modifier=[], aria=[]) -%}
<div class="fd-time{{ modifier | modifier('time') }}"{{ aria | aria }}>
{%- for item in properties.items %}
{{ time_item(item) }}
{%- endfor %}
</div>
{%- endmacro %}

{%- macro time_item(properties={}, state={}, modifier=[], classes=[], aria=[]) -%}
{%- macro time_item(properties={}) -%}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

{%- set _id = utils.id() %}
<div class="fd-time__item">
<div class="fd-time__control">
{{ button({
icon: 'navigation-up-arrow' },
modifier={ block: ['standard','light','compact'] },
aria={ label: 'Increase ' + properties.label, controls: _id },
state={
disabled: properties.lastValue
})
}}
</div>
<div class="fd-time__input">
<input class="fd-form__control" type="text" placeholder="{{properties.placeholder}}" value="{{properties.value}}" id="{{ _id }}" aria-label="{{properties.label | capitalize }}"/>
<button class="btn-playground"></button>
</div>
<input class="fd-time__input input-playground" type="text" placeholder="{{properties.placeholder}}" value="{{properties.value}}" id="{{ _id }}" aria-label="{{properties.label | capitalize }}"/>
<div class="fd-time__control">
{{ button({
icon: 'navigation-down-arrow' },
modifier={ block: ['standard','light','compact'] },
aria={ label: 'Decrease ' + properties.label, controls: _id },
state={
disabled: properties.firstValue
})
}}
<button class="btn-playground"></button>
</div>
</div>
{%- endmacro %}
21 changes: 5 additions & 16 deletions test/templates/time/data.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,38 +10,27 @@
{
"value": "02",
"placeholder": "hh",
"label": "hours",
"firstValue": false,
"lastValue": false
"label": "hours"
},
{
"value": "34",
"placeholder": "mm",
"label": "minutes",
"firstValue": false,
"lastValue": false
"label": "minutes"
},
{
"value": "56",
"placeholder": "ss",
"label": "seconds",
"firstValue": false,
"lastValue": false
"label": "seconds"
},
{
"value": "pm",
"placeholder": "am",
"label": "period",
"firstValue": false,
"lastValue": false
"label": "period"
}
]
},
"modifier": {
"block": ""
},
"state": {
"disabled": false
},
"aria": {}
}
}
60 changes: 1 addition & 59 deletions test/templates/time/index.njk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,79 +3,21 @@
{% from "./component.njk" import time %}

{% block content %}


<p>The <code>fd-time</code> component is used for a single time value. Multiple components can be used in the
<a href="time-picker"><code>fd-time-picker</code></a>
to assemble a clock time. A max of four will account for hours, minutes, seconds and period of the day.</p>

</p>

<p>
<strong>It will be rare to see this component used outside of the <code>fd-time-picker</code>.
</strong>

</p>


<h2>with values</h2>
<h2>with values and placeholders</h2>
{% set example %}
{{ time(properties=data.properties) }}
{% endset %}

{{ format(example) }}
<br><br>

<h2>with placeholder</h2>
{% set example %}
{{ time(properties={ items: [
{
"placeholder": "hh",
"label": "hours"
},
{
"placeholder": "mm",
"label": "minutes"
},
{
"placeholder": "ss",
"label": "seconds"
},
{
"placeholder": "am",
"label": "period"
}]
}) }}
{% endset %}

{{ format(example) }}

<br><br>
<h2>with button state</h2>

<p>Since the controls and inputs are standard components, they can take all states available to buttons and forms respectively, e.g., <code>disabled, .is-invalid</code>. In this case, the buttons are disabled when the first or last values are reached.
</p>


{% set example %}
{{ time(properties={ items: [
{
"value": "00",
"placeholder": "hh",
"label": "hours",
"firstValue": true
},
{
"value": "59",
"placeholder": "mm",
"label": "minutes",
"lastValue": true
}
]
}) }}
{% endset %}

{{ format(example) }}


{% endblock %}
Binary file modified ...ression_fundamental_multi-input_0_example-containerfd-multi-input_0_desktop.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified ...ges/visual_regression_fundamental_time_0_example-containerfd-time_0_desktop.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file removed ...visual_regression_fundamental_time_1_example-containerfd-time__n1_0_desktop.png
View file
Open in desktop
Binary file not shown.
Binary file removed ...visual_regression_fundamental_time_2_example-containerfd-time__n2_0_desktop.png
View file
Open in desktop
Binary file not shown.