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

fix: input-time: ignore some hierarchical mixin functionality #5079

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
+36 −21
Open

fix: input-time: ignore some hierarchical mixin functionality #5079

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7fea22b
fix: add ignore-hierarchy property to hierarchical mixin, use in inpu…
margaree Oct 17, 2024
30a5b6b
add example to show problem
margaree Oct 17, 2024
14e54c1
fix lint
margaree Oct 17, 2024
a4ce2ad
code review feedback: remove ignoreHierarchy, use data-root-view attr…
margaree Oct 18, 2024
287091d
code review feedback: add rootView property, privatize childView
margaree Oct 21, 2024
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
2 changes: 1 addition & 1 deletion components/dropdown/dropdown-menu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ class DropdownMenu extends ThemeMixin(DropdownContentMixin(LitElement)) {

// reset to root view
const menu = this.__getMenuElement();
menu.show({ preventFocus: true });
if (!menu.rootView) menu.show({ preventFocus: true });
}

_onFocus(e) {
Expand Down
9 changes: 7 additions & 2 deletions components/filter/demo/filter.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
import './filter-load-more-demo.js';
</script>
<script>
window.D2L = { LP: { Web: { UI: { Flags: { Flag: () => true } } } } };
window.D2L = { LP: { Web: { UI: { Flags: { Flag: () => false } } } } };
</script>
<meta name="viewport" content="width=device-width, minimum-scale=1, initial-scale=1.0">
<meta charset="UTF-8">
Expand Down Expand Up @@ -172,7 +172,12 @@ <h2>Date Filter</h2>
<d2l-filter-dimension-set-date-text-value key="today" range="today"></d2l-filter-dimension-set-date-text-value>
<d2l-filter-dimension-set-date-text-value key="6months" range="6months"></d2l-filter-dimension-set-date-text-value>
<d2l-filter-dimension-set-date-time-range-value key="custom" type="date"></d2l-filter-dimension-set-date-time-range-value>
<d2l-filter-dimension-set-date-time-range-value key="custom2" text="Custom Date Range with Time"></d2l-filter-dimension-set-date-time-range-value>
<d2l-filter-dimension-set-date-time-range-value key="custom2" text="Custom Date Range with Time" start-value="2024-10-12T12:00:00Z"></d2l-filter-dimension-set-date-time-range-value>
</d2l-filter-dimension-set>
<d2l-filter-dimension-set key="role" text="Role" selected-first>
<d2l-filter-dimension-set-value key="admin" text="Admin" count="0"></d2l-filter-dimension-set-value>
<d2l-filter-dimension-set-value key="instructor" text="Instructor" count="22"></d2l-filter-dimension-set-value>
<d2l-filter-dimension-set-value key="student" text="Student" count="50"></d2l-filter-dimension-set-value>
</d2l-filter-dimension-set>
</d2l-filter>
</template>
Expand Down
37 changes: 23 additions & 14 deletions components/hierarchical-view/hierarchical-view-mixin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,16 @@ export const HierarchicalViewMixin = superclass => class extends superclass {
/**
* @ignore
*/
childView: { type: Boolean, reflect: true, attribute: 'child-view' },
hierarchicalView: { type: Boolean },
/**
* @ignore
*/
hierarchicalView: { type: Boolean },
rootView: { type: Boolean, attribute: 'root-view' },
/**
* @ignore
*/
shown: { type: Boolean, reflect: true }
shown: { type: Boolean, reflect: true },
_childView: { type: Boolean, reflect: true, attribute: 'child-view' },
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 in other cases we similarly apply the underscore to the reflected property (_child-view). It's not a big deal, but when we see the attribute in the dev console it becomes extra clear that's it has been reflected internally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is used in the LMS code here so we'd want to update that somewhat semi-simultaneously if we want to make that change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha. Ok, maybe not worth it depending on your thoughts on my other comment (i.e. consumers would be able to use the public rootView instead, setting us up to eventually remove childView).

};
}

Expand Down Expand Up @@ -97,10 +98,11 @@ export const HierarchicalViewMixin = superclass => class extends superclass {
constructor() {
super();

/** @ignore */
this.childView = false;
/** @ignore */
this.hierarchicalView = true;
/** @ignore */
this.rootView = false;
this._childView = false;
this.__focusPrevious = false;
this.__intersectionObserver = null;
this.__isAutoSized = false;
Expand Down Expand Up @@ -135,7 +137,7 @@ export const HierarchicalViewMixin = superclass => class extends superclass {
}
this.__startResizeObserver();

if (!this.childView) {
if (!this._childView) {
this.addEventListener('focus', this.__focusCapture, true);
this.addEventListener('focusout', this.__focusOutCapture, true);
this.__onWindowResize = this.__onWindowResize.bind(this);
Expand Down Expand Up @@ -167,7 +169,7 @@ export const HierarchicalViewMixin = superclass => class extends superclass {

const stopPropagation = e => {
// only stop for child views, so that Esc from root view can close dropdown
if (!this.childView) return;
if (!this._childView) return;
e.stopPropagation();
};

Expand Down Expand Up @@ -198,15 +200,18 @@ export const HierarchicalViewMixin = superclass => class extends superclass {
}

getRootView() {
if (!this.childView) {
if (this.rootView || !this._childView) {
return this;
}
const rootView = findComposedAncestor(
this.parentNode,
(node) => {
return node.hierarchicalView && !node.childView;
return node.rootView || (node.hierarchicalView && !node._childView);
}
);
if (rootView) {
rootView.rootView = true;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Curious about thoughts on this - if the rootView were to change then I believe this could cause problems. For example:

<d2l-menu root-view id="menu-1">
<d2l-menu child-view id="menu-2">

but then we add another view above:

<d2l-menu id="new-menu-1">
<d2l-menu root-view id="menu-1">
<d2l-menu child-view id="menu-2">

Though I believe this would also be a problem with the current childView logic, as if something that's a child-view becomes the root-view, it would still have the child-view attribute on it, so perhaps this is a case we don't currently support.

Copy link
Contributor

@dbatiste dbatiste Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There is the method __isChildView() which is misleading. It actually updates the childView property, and is called from connectedCallback() and firstUpdated().

Kinda looks like firstUpdated call handles the initial case. Perhaps the connectedCallback call handles a scenario where the hierarchical-view is moved in the DOM (a scenario which could happen in theory, but unlikely in practice). But I think we've also had timing issues before.

I'm wondering if we should similarly initialize rootView:

  • If the consumer has specified root-view, then rootView = true
  • If the consumer has not specified root-view then we walk up the DOM...
    • If we don't find another hierarchical-view, then rootView = true
    • If we find another hierarchical-view then rootView = false

So if a view is added above, it depends on how that's done. If new DOM is being constructed to insert the element above in the DOM, then firstUpdated should handle that case. If the hierarchical-view is being moved in the DOM (ex. via removeChild and then appendChild), the connectedCallback should handle that case. In either scenario, I think we want to re-initialize rootView. It's up to the new parent to force rootView = true if it wants, and if it doesn't, rootView is re-initialized using the DOM walk.

With rootView always initialized, maybe we can update d2l-menu to use it instead of _childView.

Edit: if we do this, I propose we rename __isChildView() to __updateRootView() and set this._childView = !this.rootView.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool I think that makes sense to me. I'll update that. Thanks!

}
return rootView;
}

Expand All @@ -228,7 +233,7 @@ export const HierarchicalViewMixin = superclass => class extends superclass {
}

isActive() {
if ((this.childView && !this.shown) || !this.shadowRoot) {
if ((this._childView && !this.shown) || !this.shadowRoot) {
return false;
} else {
const content = this.shadowRoot.querySelector('.d2l-hierarchical-view-content');
Expand Down Expand Up @@ -289,7 +294,7 @@ export const HierarchicalViewMixin = superclass => class extends superclass {
}

__autoSize(view) {
if (this.__isAutoSized || this.childView) return;
if (this.__isAutoSized || this._childView) return;
requestAnimationFrame(() => {

if (view.offsetParent === null) return;
Expand Down Expand Up @@ -421,13 +426,17 @@ export const HierarchicalViewMixin = superclass => class extends superclass {
}

__isChildView() {
if (this.rootView) {
this._childView = false;
return;
}
const parentView = findComposedAncestor(
this.parentNode,
(node) => { return node.hierarchicalView; }
);

if (parentView) {
this.childView = true;
this._childView = true;
}
}

Expand Down Expand Up @@ -484,7 +493,7 @@ export const HierarchicalViewMixin = superclass => class extends superclass {
}

__onKeyDown(e) {
if (this.childView && e.keyCode === escapeKeyCode) {
if (this._childView && e.keyCode === escapeKeyCode) {
e.stopPropagation();
this.hide();
return;
Expand All @@ -498,7 +507,7 @@ export const HierarchicalViewMixin = superclass => class extends superclass {
const rootTarget = e.composedPath()[0];
if (rootTarget === this || !rootTarget.hierarchicalView) return;

if (this.childView && !this.shown) {
if (this._childView && !this.shown) {
/* deep link scenario */
this.show(e.detail.data, e.detail.sourceView);
}
Expand Down
3 changes: 2 additions & 1 deletion components/inputs/input-time.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,7 +373,8 @@ class InputTime extends InputInlineHelpMixin(FocusMixin(LabelledMixin(SkeletonMi
class="d2l-input-time-menu"
@d2l-menu-item-change="${this._handleDropdownChange}"
id="${this._dropdownId}"
role="listbox">
role="listbox"
root-view>
${menuItems}
</d2l-menu>
<div class="d2l-input-time-timezone d2l-body-small" id="${dropdownIdTimezone}" slot="footer">${this._timezone}</div>
Expand Down
6 changes: 3 additions & 3 deletions components/menu/menu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ class Menu extends ThemeMixin(HierarchicalViewMixin(LitElement)) {
changedProperties.forEach((oldValue, propName) => {
if (propName === 'label') this._labelChanged();

if (propName === 'childView' && this.childView) {
if (propName === '_childView' && this._childView) {
const items = this.shadowRoot.querySelector('.d2l-menu-items');
items.insertBefore(this._createReturnItem(), items.childNodes[0]);

Expand Down Expand Up @@ -276,7 +276,7 @@ class Menu extends ThemeMixin(HierarchicalViewMixin(LitElement)) {
return;
}

if (this.childView && e.keyCode === keyCodes.LEFT) {
if (this._childView && e.keyCode === keyCodes.LEFT) {
e.stopPropagation();
this.hide();
return;
Expand Down Expand Up @@ -343,7 +343,7 @@ class Menu extends ThemeMixin(HierarchicalViewMixin(LitElement)) {
}

_onViewResize(e) {
if (this.childView) return;
if (this._childView) return;

const eventDetails = {
bubbles: true,
Expand Down
Loading