Add d2l-input-time Component #18

svanherk · 2018-08-30T22:29:18Z

Taken from: https://github.com/BrightspaceUI/time-picker

Previous issues: https://github.com/BrightspaceUI/time-picker/issues

Will add all three to this repo

…me-picker

svanherk · 2018-08-30T22:58:32Z

Will look more into the failing galen tests tomorrow

dlockhart · 2018-08-31T12:29:48Z

d2l-input-time.html

+
+			_updateFormater: function(locale, overrides) {
+				this._setFormatter(new d2lIntl.DateTimeFormat(locale, { locale: overrides }));
+				this._setParser(new d2lIntl.DateTimeParse(locale, { locale: overrides }));


It's in this issue, but we should really switch this over to use the localize behavior's time/date formatting methods at some point.

dbatiste · 2018-08-31T12:52:06Z

d2l-input-time.html

+				--d2l-input-padding: 0.7rem 1rem;
+				--d2l-input-padding-focus: calc(0.7rem - 1px) calc(1rem - 1px);
+
+				display: inline-flex;


Is this all pre-existing code.

Having display: inline-flex; on the host worries me a little. If the consumer happens to put display: inline-block; on d2l-input-time, what affect would that have on the internal layout for the component? For specifying internal layout, I think it's better to isolate internals from the public API to avoid risk of stomping on consumer, and vice versa.

host shadow-DOM div (where flex or whatever is defined) things that flex

Thoughts?

Yup, all pre-existing code. I'm not sure the reason for inline-flex, inline-block seems to do the same thing other than 10px of padding at the bottom disappears. Feels weird making an issue for this when I could just do it now, but I'm not sure if this breaks a usage in a form somewhere

dbatiste · 2018-08-31T13:03:35Z

d2l-input-time.html

+			}
+
+			d2l-dropdown-content {
+				--d2l-dropdown-verticaloffset: 10px;


Looks like there is an option for this in the d2l-dropdown-content-behavior. Are we able to use that? I'm also wondering if we will see a pattern emerge for a smaller offset - this seems like something that we'll begin to see inconsistency on.

The comment above the property has it marked as private, so I think the intention is to set this using the css variable as above. The 20px default does seem very large and looks odd, but it probably makes more sense when there's some kind of connector or error like a menu has

I think the vertical-offset option was added to support Navbars, since the dropdown would appear too low given the specific aesthetic of those openers. The default was intended to work with buttons and the openers in the dropdown collection, and for the most part, we wanted to keep things consistent unless the case was "special"... and so the property was commented as "private" to discourage that.

In doing some poking around, there appears to be some inconsistency that's cropped up already. I suspect some of that could be the same - perhaps not 20px, but not all different either. It would be good if we could reel that in... figure out the common thread there.

Added #25 to address if we don't move to select.

dbatiste · 2018-08-31T13:05:56Z

d2l-input-time.html

+				@apply --d2l-body-compact-text;
+				color: var(--d2l-color-tungsten);
+				font-style: italic;
+				padding-top: 10px;


Seems like maybe we should follow rule of "6"s. Not sure how much of that is up for grabs though.

This is used to center the timezone text beside the dropdown, so I think that would require changes to dropdown as well?

Ah ok. It's a minor thing. If one day dropdown is updated for the design spacing changes, this would then need to be updated too. Something seemingly small that has ripple effects. :(

I misunderstood where the timezone was supposed to go, so this doesn't actually rely on the height of the dropdown. So we can change the value, but then it will cause a visual difference for anyone that pulls in the component. Are we good with that, or do we need to talk to design?

dlockhart · 2018-08-31T13:09:14Z

Since @svanherk's just transplanting these input components into d2l-inputs, for the sake of simplicity and her sanity I think we should move them in as-is and then if there are follow-up things we want to tackle or look into, let's create issues to track that.

dbatiste · 2018-08-31T13:11:46Z

d2l-input-time.html

+		</style>
+
+		<d2l-dropdown no-auto-open>
+			<div class="d2l-dropdown-opener" id="opener">


Normally I think the actual opener (the element the user will click or focus on) should have d2l-dropdown-opener class. This class is how the dropdown logic locates the focusable opener. The d2l-dropdown-opener-behavior has some logic that I think expects this, for instance to apply focus to the opener when the use hits [Escape] inside the dropdown content. There's likely a couple more little things like that. So in this case, I think it should be on the input, but perhaps there is a reason it's not in this case?

Looks like there's an issue with the scrollbar when you do this, and there's special functionality to handle scrolling what has been selected into view. I think it's related to this issue: https://github.com/BrightspaceUI/time-picker/issues/9

I've added #21 with this comment.

Yeah, I saw the scrolling bit earlier too. That's one of the behaviors that made me wonder if this should really be using your select implementation. (?) I'm surprised the opener would affect scrolling though.

Yeah, I think the select component could be a really good fit. I've been investigating a bit more but I think I'll just throw them in issues and hopefully we can just replace it later and remove those issues altogether.

dbatiste · 2018-08-31T13:18:04Z

d2l-input-time.html

+			},
+
+			attached: function() {
+				document.body.addEventListener('focus', this.__onAutoCloseFocus, true);


Do we know what's different about the focus/close logic here vs what dropdown provides? It would be really great if this component did not need to provide custom logic for this, since it's typically the source of many bugs, especially when considering shadow-DOM vs shady.

Removing this causes the menu not to disappear when someone tabs off the input without selecting anything, or clicks elsewhere on the page. It may be related to the opener not being on the right level?

Added #23

Yeah, it could be because the focusable opener element is not the one marked as the opener. Not sure. Sounds like these scenarios should be handled by dropdown though.

dbatiste · 2018-08-31T13:20:43Z

d2l-input-time.html

+			ready: function() {
+				this.__onAutoCloseFocus = this.__onAutoCloseFocus.bind(this);
+				this.__onAutoCloseClick = this.__onAutoCloseClick.bind(this);
+				this.id = this.id || ('d2l-input-time-' + instances++);


Alternatively, we have a little helper for getting unique Id. D2L.Id.getUniqueId().

dbatiste · 2018-08-31T13:25:32Z

Sounds good Dave. I wasn't sure how much clean-up was "on the table". ;)

I also wonder if this should really be using the select component that she's started on. There seems to be some behavior here that might not be exclusive to this component.

svanherk · 2018-08-31T13:43:09Z

Yeah, I also wasn't sure if we wanted to do cleanup. I was thinking we should keep breaking changes (like size and look and feel) to a minimum so its easy for others to update just doing a find/replace, and then we could do a sweep through the elements to fix issues and make another major release update. I haven't looked yet, but I expect we'll see similar issues with date and datetime.

I'll respond to the comments above and make issues accordingly. @awikkerink are you able to weigh in on any of the questions Dave has about past decisions?

svanherk · 2018-08-31T15:52:51Z

Ok merging now, I think I added all needed issues but keep discussing if I missed anything!

svanherk added 3 commits August 30, 2018 18:17

Add d2l-input-time component from https://github.com/BrightspaceUI/ti…

f0a7984

…me-picker

Fix demo

f70e51e

Add gif

260cdbf

svanherk requested a review from awikkerink August 30, 2018 22:29

svanherk requested review from dbatiste, dlockhart and margaree as code owners August 30, 2018 22:29

svanherk added 3 commits August 30, 2018 18:35

Fix up readme

9f34087

Fix file reference

7d97cda

More galen fixes

576ad62

dlockhart reviewed Aug 31, 2018

View reviewed changes

dlockhart approved these changes Aug 31, 2018

View reviewed changes

dbatiste reviewed Aug 31, 2018

View reviewed changes

Fix endpoint path

1bd5608

svanherk merged commit b043c40 into master Aug 31, 2018

svanherk deleted the Add_time_Component branch August 31, 2018 15:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add d2l-input-time Component #18

Add d2l-input-time Component #18

svanherk commented Aug 30, 2018

svanherk commented Aug 30, 2018

dlockhart Aug 31, 2018

svanherk Aug 31, 2018

dbatiste Aug 31, 2018

svanherk Aug 31, 2018 •

edited

Loading

dbatiste Aug 31, 2018

svanherk Aug 31, 2018

dbatiste Aug 31, 2018 •

edited

Loading

svanherk Aug 31, 2018

dbatiste Aug 31, 2018

svanherk Aug 31, 2018

dbatiste Aug 31, 2018

svanherk Aug 31, 2018

dlockhart commented Aug 31, 2018

dbatiste Aug 31, 2018 •

edited

Loading

svanherk Aug 31, 2018

dbatiste Aug 31, 2018

svanherk Aug 31, 2018

dbatiste Aug 31, 2018

svanherk Aug 31, 2018

dbatiste Aug 31, 2018 •

edited

Loading

dbatiste Aug 31, 2018

dbatiste commented Aug 31, 2018

svanherk commented Aug 31, 2018

svanherk commented Aug 31, 2018

Add d2l-input-time Component #18

Add d2l-input-time Component #18

Conversation

svanherk commented Aug 30, 2018

svanherk commented Aug 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svanherk Aug 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbatiste Aug 31, 2018 • 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

dlockhart commented Aug 31, 2018

dbatiste Aug 31, 2018 • 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

dbatiste Aug 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbatiste commented Aug 31, 2018

svanherk commented Aug 31, 2018

svanherk commented Aug 31, 2018

svanherk Aug 31, 2018 •

edited

Loading

dbatiste Aug 31, 2018 •

edited

Loading

dbatiste Aug 31, 2018 •

edited

Loading

dbatiste Aug 31, 2018 •

edited

Loading