-
Notifications
You must be signed in to change notification settings - Fork 59
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.
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
feat: add self-contained styles for Time component #269
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
@import "../scss/components/time"; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,17 +11,17 @@ $block: #{$fd-namespace}-time; | |
max-width: fd-space(10); | ||
text-align: center; | ||
margin-right: fd-space(1); | ||
|
||
&:last-child { | ||
margin-right: 0; | ||
} | ||
} | ||
|
||
&__input { | ||
// Spacing and narrower side padding for child inputs | ||
.#{$block}__input { | ||
margin: fd-space(2) 0; | ||
input { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 &__input {
margin: fd-space(2) 0;
padding-left: fd-space(2);
padding-right: fd-space(2);
text-align: center;
} I would also put the This will all be a breaking change, but hey -- we're on version 0. 😄 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,7 +25,8 @@ body { | |
} | ||
|
||
.input-playground { | ||
width: 250px; | ||
width: 100%; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
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={}) -%} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 %} |
There was a problem hiding this comment.
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 offd-time
withinfd-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 thefd-time__item
blocks to prevent them from ever "wrapping". Thoughts?There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.