Skip to content
This repository has been archived by the owner on Aug 30, 2018. It is now read-only.

switch cart over to table layout #412

Merged
merged 6 commits into from
May 20, 2015
Merged

switch cart over to table layout #412

merged 6 commits into from
May 20, 2015

Conversation

stevebosworth
Copy link
Contributor

@cshold @mpiotrowicz

Switched the cart over to a table layout for improved a11y

Only defined for IE9+
==============================================================================*/
html:not(.lt-ie9) {
.table--responsive {
Copy link
Contributor

Choose a reason for hiding this comment

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

there are more IE9 + browsers than not, why position this as the exception?

Copy link
Contributor

Choose a reason for hiding this comment

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

If responsive tables are default, we'd have to undo all the styles for older IE. This covers all modern browsers since they won't match the :not selector with the least styles defined. Open to suggestions for sure though

Copy link
Contributor

Choose a reason for hiding this comment

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

but since they're under a media query these styles wouldn't apply to oldIE anyway?

Copy link
Contributor

Choose a reason for hiding this comment

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

With respond.js all media queries are processed too. First time it wasn't really wanted.

Copy link
Contributor

Choose a reason for hiding this comment

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

so then why not add the responsive behavior to oldIE?

Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly I thought it would be totally busted in IE8, even with respond.js. This demo proves that wrong. @stevebosworth, can you get a demo shop up with this PR and remove the :not(.lt-ie9) - it may work perfectly already :)

Copy link
Contributor

Choose a reason for hiding this comment

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

great lesson in never assuming anything :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice! It totally works on ie8 without the conditional

@stevebosworth
Copy link
Contributor Author

just waiting on translated strings for i18n but good to go after that?

@cshold
Copy link
Contributor

cshold commented May 14, 2015

Yup, looks good on my end

stevebosworth added a commit that referenced this pull request May 20, 2015
switch cart over to table layout
@stevebosworth stevebosworth merged commit f021516 into master May 20, 2015
@stevebosworth stevebosworth deleted the cart-fix-tables branch May 20, 2015 18:05
@suture
Copy link

suture commented Jun 3, 2015

Just looking at the source on this page
https://timber-demo.myshopify.com/products/bjml-mystic-water-clean

<span class="visually-hidden">Regular price</span>
<span id="ProductPrice" class="h2" itemprop="price">$257.40</span>

<span class="visually-hidden">Sale price</span>
<p id="ComparePrice">Compare at $365.99</p>

The visually hidden "Sale price" is listed with the higher "Compare at" price.
Is that correct? Shouldn't it be the other way round.

Maybe the terminology is different in different countries, or maybe I misunderstand. But normally from my experience the sale price should be cheaper.

@Cam
Copy link

Cam commented Jun 3, 2015

@suture I think you are right. It should be something like "Original price"

@cshold
Copy link
Contributor

cshold commented Jun 3, 2015

@stevebosworth you able to make a quick update for that?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants