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.

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

Change separator lines opacity #393

Merged
merged 4 commits into from
Aug 25, 2021
Merged

Change separator lines opacity #393

merged 4 commits into from
Aug 25, 2021

Conversation

ludoboludo
Copy link
Contributor

@ludoboludo ludoboludo commented Aug 11, 2021

Why are these changes introduced?

Fixes #22

What approach did you take?

Changed some of the existing value to be 0.08 instead of 0.2.

Other considerations

Demo links

Checklist

@ludoboludo ludoboludo marked this pull request as ready for review August 12, 2021 15:19
@kmeleta kmeleta self-requested a review August 19, 2021 14:41
Copy link
Contributor

@kmeleta kmeleta left a comment

Choose a reason for hiding this comment

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

Couple quick questions

  1. Should the footer borders be updated here as well? https://screenshot.click/19-48-rt6fv-c6p0b.png

  2. I'd kind of expect the cart items to receive this change as well https://screenshot.click/19-45-fjcs7-a28o1.png. There may be a reason this wasn't called out in the ticket but wanted to float that.

@martinamarien
Copy link
Contributor

Screen Shot 2021-08-19 at 1 02 25 PM

On shops with light backgrounds, the line contrast is more defined. On darker backgrounds, the line is almost invisible. In the screenshot I have provided, there are lines around the product cards and a separator line below the header. Are we sure we want to change the line opacity to 8%?

@martinamarien martinamarien mentioned this pull request Aug 19, 2021
5 tasks
@ludoboludo
Copy link
Contributor Author

Screen Shot 2021-08-19 at 1 02 25 PM

On shops with light backgrounds, the line contrast is more defined. On darker backgrounds, the line is almost invisible. In the screenshot I have provided, there are lines around the product cards and a separator line below the header. Are we sure we want to change the line opacity to 8%?

Oof, good point 🤔 cc: @Guillaumegranger1

@Guillaumegranger1
Copy link

I know it makes the theme less scalable, but I think we should move forward with it because it was requested by stakeholders and I don't feel like fighting that battle. Or can we implement the update as is, and see later if we can slightly bump up the opacity? Is it easy to do? ...like changing a variable in one place? @ludoboludo

@ludoboludo
Copy link
Contributor Author

Yes, it wouldn't be hard to change later. It's not something in one place but a few. Doesn't take long to figure out which ones though.

chrisberthe
chrisberthe previously approved these changes Aug 24, 2021
Copy link
Contributor

@chrisberthe chrisberthe left a comment

Choose a reason for hiding this comment

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

Looks good. @melissaperreault would the cart notification border be part of this colour update as well? I noticed this variation so questioning it here:

image

@kmeleta kmeleta self-requested a review August 24, 2021 17:06
kmeleta
kmeleta previously approved these changes Aug 24, 2021
Copy link
Contributor

@kmeleta kmeleta left a comment

Choose a reason for hiding this comment

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

If we're moving forward with this change as is, LGTM

@ludoboludo
Copy link
Contributor Author

Looks good. @melissaperreault would the cart notification border be part of this colour update as well? I noticed this variation so questioning it here:

image

I assumed we didn't want to do it based on this: screenshot from the issue. I had tested and it looked pretty faint in contrast with the background but I see how it's not seemless with the rest of the header border

@melissaperreault
Copy link
Contributor

Looks good. @melissaperreault would the cart notification border be part of this colour update as well? I noticed this variation so questioning it here:

I assumed we didn't want to do it based on this: screenshot from the issue. I had tested and it looked pretty faint in contrast with the background but I see how it's not seemless with the rest of the header border

I actually find it detaches and help emphasize the component so I would keep it as is 👌 We also have the interest to revise the overlay patterns across similar experience from this issue #462

Copy link
Contributor

@melissaperreault melissaperreault left a comment

Choose a reason for hiding this comment

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

Thanks Ludo, took a look and looks good!

I think we could also make the change in My account tables to have a similar feeling from the Cart. Example of the outcome.

This would affect the Account order history and the order detail tables I believe.

@ludoboludo
Copy link
Contributor Author

@melissaperreault I was making the changes and thought I'd bring something up. It has some continuity when doing it, so it matches the footer and header but the role of the borders here also plays a role in terms of accessibility.
It helps keep track of which order you're looking at. So if the lines are pretty faint it will make it harder 🤔
So it might be worth keeping it as is.
Let me know what you think, I can loop Scott in if necessary 👍

@melissaperreault
Copy link
Contributor

melissaperreault commented Aug 25, 2021

It has some continuity when doing it, so it matches the footer and header but the role of the borders here also plays a role in terms of accessibility.

I understand the point you raise but believe it would be aligned with the overall changes and minimal aesthetic to harmonize in those templates too. To me, the borders are already not accessible (low contrast) at 20% and we have even fewer separator in the Cart template which wears the 8% already.

This might be a bigger conversation whether our table are easy to consult through Fable testing to inform how we can improve legibility, but it would require probably more than increasing the opacity of the borders.

I was not part of the effort to change some of the border opacity so I'd like to loop in @Oliviammarcello and @Guillaumegranger1 , perhaps they did consider keeping them at 20%? 🙏

@ludoboludo ludoboludo dismissed stale reviews from kmeleta and chrisberthe via 082308d August 25, 2021 14:27
Copy link
Contributor

@melissaperreault melissaperreault left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉 🙏

@ludoboludo
Copy link
Contributor Author

@kmeleta & @chrisberthe I'd need your approval again. I just changed the opacity on the account page for the table or order page

@Guillaumegranger1
Copy link

Yes! 🚢🚀

@ludoboludo ludoboludo merged commit 3d6f652 into main Aug 25, 2021
@ludoboludo ludoboludo deleted the separator-line-opacity branch August 25, 2021 15:51
@metamoni metamoni mentioned this pull request Mar 7, 2023
15 tasks
phapsidesGT pushed a commit to Gravytrain-UK/gt-shopify-dawn-theme that referenced this pull request Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update the opacity of all separator lines
6 participants