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

Remove all instances of optional chaining #489

Merged
merged 5 commits into from
Aug 27, 2021
Merged

Remove all instances of optional chaining #489

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6e2e208
remove all instances of optional chaining
martinamarien Aug 25, 2021
4c847f3
fix reviewer comments
martinamarien Aug 26, 2021
ddc08a6
fix formtting
martinamarien Aug 26, 2021
bfcfee2
fix youtube media on mobile viewports
martinamarien Aug 27, 2021
360e4c8
fix spacing
martinamarien Aug 27, 2021
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 assets/cart-notification.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ class CartNotification extends HTMLElement {
this.getSectionInnerHTML(parsedState.sections[section.id], section.selector);
}));

this.header?.reveal();
if (this.header) this.header.reveal();
this.open();
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't get the cart notification to show up 🤔 on Safari 12.1

Copy link
Contributor Author

@martinamarien martinamarien Aug 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Really? I just tested it, and I'm able to get it to work with 12.1.

Screen.Recording.2021-08-26.at.4.44.37.PM.mov

Copy link
Contributor

Choose a reason for hiding this comment

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

If it worked for you then it should be good 👍 I will test again

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Ludo

Copy link
Contributor

Choose a reason for hiding this comment

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

Confirmed some of those issues and created a follow up issue here: #510

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you 🙏🏻

}

Expand Down
7 changes: 5 additions & 2 deletions assets/cart.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,9 @@ class CartItems extends HTMLElement {
.then((state) => {
const parsedState = JSON.parse(state);
this.classList.toggle('is-empty', parsedState.item_count === 0);
document.getElementById('main-cart-footer')?.classList.toggle('is-empty', parsedState.item_count === 0);
const cartFooter = document.getElementById('main-cart-footer');

if (cartFooter) cartFooter.classList.toggle('is-empty', parsedState.item_count === 0);

this.getSectionsToRender().forEach((section => {
const elementToReplace =
Expand All @@ -83,7 +85,8 @@ class CartItems extends HTMLElement {
}));

this.updateLiveRegions(line, parsedState.item_count);
document.getElementById(`CartItem-${line}`)?.querySelector(`[name="${name}"]`)?.focus();
const lineItem = document.getElementById(`CartItem-${line}`);
if (lineItem) lineItem.querySelector(`[name="${name}"]`).focus();
this.disableLoading();
}).catch(() => {
this.querySelectorAll('.loading-overlay').forEach((overlay) => overlay.classList.add('hidden'));
Expand Down
9 changes: 6 additions & 3 deletions assets/collection-filters-form.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ class CollectionFiltersForm extends HTMLElement {
}

onHistoryChange(event) {
const searchParams = event.state?.searchParams || '';
const searchParams = event.state ? event.state.searchParams : '';
this.renderPage(searchParams, null, false);
}

Expand Down Expand Up @@ -97,9 +97,12 @@ class CollectionFiltersForm extends HTMLElement {

const facetDetailsElements =
parsedHTML.querySelectorAll('#CollectionFiltersForm .js-filter, #CollectionFiltersFormMobile .js-filter');
const matchesIndex = (element) => element.dataset.index === event?.target.closest('.js-filter')?.dataset.index
const matchesIndex = (element) => {
const jsFilter = event ? event.target.closest('.js-filter') : undefined;
return element.dataset.index === jsFilter.dataset.index
}
const facetsToRender = Array.from(facetDetailsElements).filter(element => !matchesIndex(element));
const countsToRender = Array.from(facetDetailsElements).find(matchesIndex);
const countsToRender = Array.from(facetDetailsElements).find(matchesIndex);

facetsToRender.forEach((element) => {
document.querySelector(`.js-filter[data-index="${element.dataset.index}"]`).innerHTML = element.innerHTML;
Expand Down
35 changes: 24 additions & 11 deletions assets/global.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ function pauseAllMedia() {
video.contentWindow.postMessage('{"method":"pause"}', '*');
});
document.querySelectorAll('video').forEach((video) => video.pause());
document.querySelectorAll('product-model').forEach((model) => model.modelViewerUI?.pause());
document.querySelectorAll('product-model').forEach((model) => { if(model.modelViewerUI) modelViewerUI.pause()});
}

function removeTrapFocus(elementToFocus = null) {
Expand Down Expand Up @@ -394,9 +394,10 @@ class ModalDialog extends HTMLElement {

show(opener) {
this.openedBy = opener;
const popup = this.querySelector('.template-popup');
document.body.classList.add('overflow-hidden');
this.setAttribute('open', '');
this.querySelector('.template-popup')?.loadContent();
popup.loadContent();
trapFocus(this, this.querySelector('[role="dialog"]'));
}

Expand All @@ -414,8 +415,11 @@ class ModalOpener extends HTMLElement {
super();

const button = this.querySelector('button');
button?.addEventListener('click', () => {
document.querySelector(this.getAttribute('data-modal'))?.show(button);

if (!button) return;
button.addEventListener('click', () => {
const modal = document.querySelector(this.getAttribute('data-modal'))
modal.show(button);
});
}
}
Expand All @@ -424,7 +428,9 @@ customElements.define('modal-opener', ModalOpener);
class DeferredMedia extends HTMLElement {
constructor() {
super();
this.querySelector('[id^="Deferred-Poster-"]')?.addEventListener('click', this.loadContent.bind(this));
const poster = this.querySelector('[id^="Deferred-Poster-"]');
if (!poster) return;
poster.addEventListener('click', this.loadContent.bind(this));
}

loadContent() {
Expand Down Expand Up @@ -537,7 +543,8 @@ class VariantSelects extends HTMLElement {
}

updateMedia() {
if (!this.currentVariant || !this.currentVariant?.featured_media) return;
if (!this.currentVariant) return;
if (!this.currentVariant.featured_media) return;
const newMedia = document.querySelector(
`[data-media-id="${this.dataset.section}-${this.currentVariant.featured_media.id}"]`
);
Expand Down Expand Up @@ -572,7 +579,7 @@ class VariantSelects extends HTMLElement {
const pickUpAvailability = document.querySelector('pickup-availability');
if (!pickUpAvailability) return;

if (this.currentVariant?.available) {
if (this.currentVariant && this.currentVariant.available) {
pickUpAvailability.fetchAvailability(this.currentVariant.id);
} else {
pickUpAvailability.removeAttribute('available');
Expand All @@ -591,13 +598,17 @@ class VariantSelects extends HTMLElement {

if (source && destination) destination.innerHTML = source.innerHTML;

document.getElementById(`price-${this.dataset.section}`)?.classList.remove('visibility-hidden');
const price = document.getElementById(`price-${this.dataset.section}`);

if (price) price.classList.remove('visibility-hidden');
this.toggleAddButton(!this.currentVariant.available, window.variantStrings.soldOut);
});
}

toggleAddButton(disable = true, text, modifyClass = true) {
const addButton = document.getElementById(`product-form-${this.dataset.section}`)?.querySelector('[name="add"]');
const productForm = document.getElementById(`product-form-${this.dataset.section}`);
if (!productForm) return;
const addButton = productForm.querySelector('[name="add"]');

if (!addButton) return;

Expand All @@ -613,10 +624,12 @@ class VariantSelects extends HTMLElement {
}

setUnavailable() {
const addButton = document.getElementById(`product-form-${this.dataset.section}`)?.querySelector('[name="add"]');
const button = document.getElementById(`product-form-${this.dataset.section}`);
const addButton = button.querySelector('[name="add"]');
const price = document.getElementById(`price-${this.dataset.section}`);
if (!addButton) return;
addButton.textContent = window.variantStrings.unavailable;
document.getElementById(`price-${this.dataset.section}`)?.classList.add('visibility-hidden');
if (price) price.classList.add('visibility-hidden');
}

getVariantData() {
Expand Down
5 changes: 3 additions & 2 deletions assets/pickup-availability.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
if (!customElements.get('pickup-availability')) {
customElements.define('pickup-availability', class PickupAvailability extends HTMLElement {
constructor() {
super();
super();

if(!this.hasAttribute('available')) return;

Expand All @@ -22,7 +22,8 @@ if (!customElements.get('pickup-availability')) {
this.renderPreview(sectionInnerHTML);
})
.catch(e => {
this.querySelector('button')?.removeEventListener('click', this.onClickRefreshList);
const button = this.querySelector('button');
if (button) button.removeEventListener('click', this.onClickRefreshList);
this.renderError();
});
}
Expand Down
2 changes: 1 addition & 1 deletion assets/product-model.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,4 +53,4 @@ window.ProductModel = {
},
};

window.addEventListener('DOMContentLoaded', () => { window.ProductModel?.loadShopifyXR(); });
window.addEventListener('DOMContentLoaded', () => { if (window.ProductModel) window.ProductModel.loadShopifyXR(); });
2 changes: 1 addition & 1 deletion assets/share.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ if (!customElements.get('share-button')) {
successMessage: this.querySelector('[id^="ShareMessage"]'),
urlInput: this.querySelector('input')
}
this.urlToShare = this.elements.urlInput?.value || document.location.href;
this.urlToShare = this.elements.urlInput ? this.elements.urlInput.value : document.location.href;

if (navigator.share) {
this.mainDetailsToggle.setAttribute('hidden', '');
Expand Down
3 changes: 2 additions & 1 deletion sections/featured-product.liquid
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -414,13 +414,14 @@
}
)
const activeMedia = this.querySelector(`[data-media-id="${this.openedBy.getAttribute("data-media-id")}"]`);
const activeMediaContent = activeMedia.querySelector('content');
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar comment here to what Chris mentioned on the main-product.liquid file. Though it's switching to the active media properly 🤔 the activeMediaContent returns a null value when I log it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ActiveMediaContent is only being used for youtube videos it seems.
But I am actually unable to output any console messages when interacting with videos and 3D modals on the product page or in the modal.

@LucasLacerdaUX now that videos and modals are not being used to open the product modal, do we still need this code?

Copy link
Contributor

@LucasLacerdaUX LucasLacerdaUX Aug 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@LucasLacerdaUX now that videos and modals are not being used to open the product modal, do we still need this code?

They are still being used to open the modal, on mobile. I'm not sure if it is also true for featured product, tho. I'll check.

But why is this querySelector targeting a content element? In the original code, we are targeting the template and then retrieving its content (as in a property, not an actual element with the tag content). I haven't reviewed the full code so ignore my comment if it doesn't make any sense :P

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've tried both, and they didn't seem to make a difference, but I'll check specifically on mobile. It's possible I didn't test on an actual device. I forgot that on mobile, they are are all at as openers for the modal.

activeMedia.classList.add('active');
activeMedia.scrollIntoView();

const container = this.querySelector('[role="document"]');
container.scrollLeft = (activeMedia.width - container.clientWidth) / 2;

if (activeMedia.nodeName == 'DEFERRED-MEDIA' && activeMedia.querySelector('template')?.content?.querySelector('.js-youtube'))
if (activeMedia.nodeName == 'DEFERRED-MEDIA' && activeMediaContent && activeMediaContent.querySelector('.js-youtube'))
activeMedia.loadContent();
}
});
Expand Down
3 changes: 2 additions & 1 deletion sections/footer.liquid
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,8 +342,9 @@

onItemClick(event) {
event.preventDefault();
const form = this.querySelector('form');
this.elements.input.value = event.currentTarget.dataset.value;
this.querySelector('form')?.submit();
if (form) form.submit();
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't test this part as the country/region and language selectors wouldn't open

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 was able to test and it seems to be working on 12.1 for me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Created an issue for follow up: #510

}

openSelector() {
Expand Down
7 changes: 4 additions & 3 deletions sections/main-product.liquid
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -466,14 +466,15 @@
}
)
const activeMedia = this.querySelector(`[data-media-id="${this.openedBy.getAttribute("data-media-id")}"]`);
const activeMediaContent = activeMedia.querySelector('content');
Copy link
Contributor

Choose a reason for hiding this comment

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

Think there's something off here—I believe it should be updated to template and then we need to check for the .content property:

Suggested change
const activeMediaContent = activeMedia.querySelector('content');
const activeMediaContent = activeMedia.querySelector('template');

activeMedia.classList.add('active');
activeMedia.scrollIntoView();

const container = this.querySelector('[role="document"]');
container.scrollLeft = (activeMedia.width - container.clientWidth) / 2;
container.scrollLeft = (activeMedia.width - container.clientWidth) / 2;

if (activeMedia.nodeName == 'DEFERRED-MEDIA' && activeMedia.querySelector('template')?.content?.querySelector('.js-youtube'))
activeMedia.loadContent();
if (activeMedia.nodeName == 'DEFERRED-MEDIA' && activeMediaContent && activeMediaContent.querySelector('.js-youtube'))
activeMedia.loadContent();
}
}

Expand Down