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 white space mobile view for product page #655

Merged
merged 7 commits into from
Sep 24, 2021
Merged

Remove white space mobile view for product page #655

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f444f26
Remove white space mobile view for product page
sofiamatulis Sep 20, 2021
75a15a0
Remove visibility hidden
sofiamatulis Sep 20, 2021
87d531a
Add display none when editor mode
sofiamatulis Sep 21, 2021
73e2c44
Update spacing for mobile
sofiamatulis Sep 22, 2021
9e5da6c
fix align
sofiamatulis Sep 22, 2021
cb84008
Revert
sofiamatulis Sep 23, 2021
f4843fe
Update specifity class;
sofiamatulis Sep 23, 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
5 changes: 3 additions & 2 deletions assets/component-product-model.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
.button.product__xr-button {
.product__xr-button {
background: rgba(var(--color-foreground), 0.08);
color: rgb(var(--color-foreground));
margin: 1rem auto;
box-shadow: none;
display: flex;
Copy link
Contributor

@LucasLacerdaUX LucasLacerdaUX Sep 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This is overriding the display: none we added below:

Can you please verify if we need the .button before the .product__xr-button? I don't remember why I added it in the first place, maybe we had some conflict / specificity issue before.

EDIT: Now I remember why I added this .button. It was being used to override the hover style, so I believe you don't need it on the main selector and can update it to look like that (and ignore the suggestion in my next comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

It still seems to hide it properly with display: none !important applied to it from the hidden class. From here:

So maybe we just don't need that other display none 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

That JS solution is only applied on Design Mode. The issue I mentioned above, related to the CSS property, is applied both inside and outside the Editor.

Before / After (outside the Theme Editor)

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 can see needing the class button because we have a modifier class right after 🤔 button--full-width

Good catch on the flex. I am reverting that and will try to align it on the ipad breakdown differently

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah gotcha, I was focusing on the Editor 🤦 but I see it on the "live" version 👍 good catch.

Copy link
Contributor

@LucasLacerdaUX LucasLacerdaUX Sep 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, I wasn't suggesting to remove the button class from the element. Sorry, that was a bit confusing on my first comment.

My suggestion was to reduce the specificity from the CSS selector (except on :hover):

If not possible, then you can apply the solution in my next comment below - to increase the specificity on the selector inside the media query.

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 apologize - I misunderstood your comment 😅 I removed the specificity on product__xr-button and kept the display flex and it seems to work on all of the scenarios: 1) mobile screens on desktop 2) editor view on mobile 3) actual devices on mobile and tablet

}

.button.product__xr-button:hover {
Expand All @@ -14,7 +15,7 @@
}

@media screen and (max-width: 749px) {
slider-component .product__xr-button:not([data-shopify-xr-hidden]) {
slider-component .product__xr-button {
display: none;
}

Expand Down
3 changes: 3 additions & 0 deletions assets/product-model.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,5 +54,8 @@ window.ProductModel = {
};

window.addEventListener('DOMContentLoaded', () => {
if (Shopify.designMode) {
document.querySelectorAll("[data-shopify-xr-hidden]").forEach(element => element.classList.add('hidden'));
Copy link
Contributor

Choose a reason for hiding this comment

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

For this Editor Mobile view, I feel that second button could be displayed inside the editor instead of adding display: none to it. This would allow the merchant to see and tweak its colors.

The weird part about it, though, is that merchants wouldn't be able to launch the actual XR (so the button would act as disabled). AFAIK, the shopify-xr script does not work in Editor mode.

I’m also okay with this current solution as it matches the decision we previously made (to have a whitespace).

Copy link
Contributor Author

@sofiamatulis sofiamatulis Sep 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I had a conversation with Ludo on this and I feel like it makes sense to keep it hidden on the browser since this can only be used on the actual device (and it wouldnt work). This way we are also consistent with other themes.

@melissaperreault Let me know your thoughts - we can always create a follow up enhancement issue

Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to show the button and then use some sort of notification or alert to explain to the merchant that on a mobile device this would activate "view in your space" ?

Copy link
Contributor

@ludoboludo ludoboludo Sep 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

But then not all mobile device support the view in your space thing right ? So we also need that mentioned.
Unless it shows on any device and takes you to the app store to use a compatible app to view it 🤷

Copy link
Contributor

@melissaperreault melissaperreault Sep 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@LucasLacerdaUX is bringing a good point.

We'll need to keep monitoring merchant's experience when they want to validate how the feature will work/display for them and their customers when editing their theme. If they don't see any button in the editor view, regardless if it is compatible or not, they might think it is a bug and could contact support advisor, etc. I am surprised we don't have any insights on that experience yet, isn't it something that was happening on Debut and Narrative already?

On the other side, we don't want to show an invalid layout to merchants as they would again perceive it as a bug because they wouldn't be able to compare what is the reality for the same reasons.

A potential missed opportunity is to have a fallback CTA label/tooltip in place that informs buyers there is a 3D model available and that they should consider viewing this product on the proper device to have the right experience. cc. @bldelgado Do you recall if this was explored? 🤔

For the short term, I don't mind having this fix until we find the right experience, but in the meantime, I would hope that merchants help docs could inform merchants that they could never see the View in your space button in the editor mode. Thoughts? (I am not sure where in the help docs this information could live, at the product media type level but perhaps also at the theme customization detail level?)

This would allow the merchant to see and tweak its colors

I don't think this is the biggest concern as the theme color scheme doesn't allow you to tweak granular colors like that.

Copy link
Member

Choose a reason for hiding this comment

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

But then not all mobile device support the view in your space thing right ? So we also need that mentioned.
Unless it shows on any device and takes you to the app store to use a compatible app to view it

Most Android/Apple devices will support it. It does not require an app and uses the native functionality of iOS Quicklook or Android Scene Viewer if the device is new enough.

}
if (window.ProductModel) window.ProductModel.loadShopifyXR();
});