-
Notifications
You must be signed in to change notification settings - Fork 25
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
Updating package-lock.json #5158
base: main
Are you sure you want to change the base?
Updating package-lock.json #5158
Conversation
Thanks for the PR! 🎉 We've deployed an automatic preview for this PR - you can see your changes here:
Note The build needs to finish before your changes are deployed. |
7732602
to
7babf84
Compare
@@ -628,7 +628,7 @@ export const DropdownContentMixin = superclass => class extends LocalizeCoreElem | |||
|
|||
// DE44538: wait for dropdown content to fully render, | |||
// otherwise this.getContentContainer() can return null. | |||
await this.updateComplete; | |||
await this.__waitForContentContainer(); |
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.
@dbatiste @margaree FYI on the changes in here.
This first one wasn't causing vdiffs to fail, but it was causing a lot of errors in the console because this.getContentContainer()
was returning null
and the code below was assuming that it wasn't. Clearly based on this comment this was a problem in the past, but I'm guessing waiting for updateComplete
is no longer enough.
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 wonder why that element would not be rendered by the time updateComplete
is fulfilled.
e.preventDefault(); | ||
await this._waitForItems(); |
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 was seeing consistent vdiff results where it wasn't focusing on the first item in the menu when the time input was opened. I tracked this down to the menu bailing on its focus code because there weren't any items and it therefore couldn't determine where to move focus. That's because time inputs don't render their menu items until the menu is first opened as a performance thing.
So this change sets this._dropdownFirstOpened
to true
which will cause a re-render and the items to be rendered. It then waits for them to be present in the DOM, and only then does it set opened
which will cause another re-render and the menu code will take over to place focus.
@@ -167,7 +167,8 @@ describe('d2l-input-date-time', () => { | |||
it('open time', async() => { | |||
const elem = await fixture(basicFixture); | |||
const textInput = elem.shadowRoot.querySelector('d2l-input-time').shadowRoot.querySelector('input'); | |||
await sendKeysElem(textInput, 'press', 'Enter'); | |||
sendKeysElem(textInput, 'press', 'Enter'); | |||
await oneEvent(elem, 'd2l-dropdown-open'); |
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.
It needs to wait longer now.
cda31ce
to
0f24d88
Compare
The line-wrapping diffs are expected (Chrome now matches Firefox and Safari) but I'll spend some more time investigating the other stuff. It could be a repercussion of delaying dropdown opening a bit so things have resized. |
0f24d88
to
842d6b0
Compare
153d2f3
to
ea9fee5
Compare
@@ -49,11 +49,11 @@ function addSpaceListener() { | |||
if (spaceListenerAdded) return; | |||
spaceListenerAdded = true; | |||
document.addEventListener('keydown', e => { | |||
if (e.keyCode !== 32) return; | |||
if (e.key !== ' ') return; |
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.
My IDE was giving me warnings since keyCode
is deprecated. This is functionally equivalent.
@@ -617,7 +618,7 @@ class Filter extends FocusMixin(LocalizeCoreElement(RtlMixin(LitElement))) { | |||
return html` | |||
<d2l-list-item | |||
id="${itemId}" | |||
@d2l-list-item-selected="${ifDefined(item.additionalContent ? this._handleListItemSelelcted : undefined)}" | |||
@d2l-list-item-selected="${item.additionalContent ? this._handleListItemSelected : undefined}" |
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.
Wrapping this in ifDefined
does nothing since that's just for attributes. Also fixing typo in the handler name.
_handleListItemSelelcted() { | ||
if (hasDisplayedKeyboardTooltip || !spacePressed) return; | ||
this._displayKeyboardTooltip = true; | ||
hasDisplayedKeyboardTooltip = true; |
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.
We were getting really consistent failures (~90% of the time) where the tooltip which is tied to the list item would position itself before the expand/collapse area had finished opening. This captures whether spacebar was pressed (because we lose that information after) but moves the displaying of the tooltip up into the event handler for expand/collapse.
b132e40
to
3bff7b8
Compare
3868c87
to
a6dc099
Compare
* Updating vdiff goldens * restore --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Dave Lockhart <[email protected]>
a6dc099
to
faff273
Compare
Automatic update of the
package-lock.json
file.Dependency Changes