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

Add a "Show separator lines" option to the footer #1776

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

Add a "Show separator lines" option to the footer #1776

wants to merge 19 commits into from
+72 −11

Conversation

kjellr
Copy link
Contributor

@kjellr kjellr commented Jun 9, 2022 •
edited
Loading

PR Summary:

Added a "Show separator lines" option to the footer.

separator-lines

Why are these changes introduced?

We currently offer this option in the header, but we do not offer it in the footer. This feels inconsistent. 🙂

What approach did you take?

  • I added a setting to the Footer area, mimicking the approach taken for the header (adding a class, etc.).
  • I chose to use a single setting to control both separators in the footer. I thought this made sense since the two separators are already linked today (if you have no content in the top footer area, the bottom separator disappears).

Other considerations

  • This setting is really only relevant if "Background 1" is chosen as the background. It would be cool to make this new setting show up conditionally only if that is the selected color (The header should probably work this way too, since it's also only relevant if "Background 1" is selected).

I'd love feedback on these three items specifically:

  • I'm not sure about combining the two separators into one option, but adding two options felt like overkill. 🤷 What do you all think?
  • I wasn't entirely sure where to place the new option, so I just placed it below the background color option. Do folks think this makes sense?
  • I updated the default English locale file to reflect that this controls two separators, not just one. I'm honestly not sure how to generate all the other locale files (this is my first PR, and I just manually updated those! 🙈). The rest all still refer to just a single "separator" and need to be updated.

Testing steps/scenarios

  • Visit this link
  • Select the Footer
  • Toggle the new "Show separator lines" option.

Checklist

@kjellr kjellr added the Category: Enhancement New feature or request label Jun 9, 2022
@kjellr kjellr self-assigned this Jun 9, 2022
@kjellr
Copy link
Contributor Author

kjellr commented Jun 9, 2022 •
edited
Loading

I updated the default English locale file to reflect that this controls two separators, not just one. I'm honestly not sure how to generate all the other locale files (this is my first PR, and I just manually updated those! 🙈). The rest all still refer to just a single "separator" and need to be updated.

Judging by the failed translation-platform check here, I'm guessing I should actually revert all those non-english translations, and then we'll cross that translation bridge later.

ludoboludo
ludoboludo reviewed Jun 20, 2022
View reviewed changes
Copy link
Contributor

@ludoboludo ludoboludo left a comment

Choose a reason for hiding this comment

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

Looking good 👌 Added a few comments

config/settings_data.json Outdated Show resolved Hide resolved
sections/footer.liquid Outdated Show resolved Hide resolved
assets/section-footer.css
Comment on lines -5 to -7
.footer:not(.color-background-1) {
border-top: none;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This means that it would have a visual impact on merchants' stores potentially where if they were using a background-1 color scheme it would hide the separator lines. Or the other way around if we set them to true by default.
So any merchant using a color scheme other than background-1 will now get some separator lines showing.

Not a big issue, just wanting to flag this as something that we will need to be aware when doing new releases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good call. I missed that. Restored in 3ef6fe6

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 it shouldn't have the :not(.color-background-1) part in the selector 🤔

Otherwise when I have a background footer I don't have a border top on the footer:

Sorry if what I mentioned was not clear.
I just meant that this will have a visual impact on their store most likely but that it's ok. It's going to be part of this update 👍

It will be something to keep in mind for us devs next time we do releases and set the show separator lines according to each theme we have. So that for the ones that have a footer not using color-background-1 we will set that new setting to false to keep it similar to before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah okay. Hmm. I'll revert that last commit for now then.

It's a tough one — I don't think it's great to make changes to users' existing sites, but I also think that the visual change makes a lot of sense in the context of the new control. 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. Sometimes there is no other option than having a visual change to be able to provide more control. 👍

assets/section-footer.css Outdated Show resolved Hide resolved
ludoboludo
ludoboludo previously approved these changes Jun 29, 2022
View reviewed changes
Copy link
Contributor

@ludoboludo ludoboludo 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 to me 👌 🙂

@kjellr
Copy link
Contributor Author

kjellr commented Jul 1, 2022 •
edited
Loading

I just reverted the incorrect labels that I'd manually added in 51940de, back before I understood how translations work. Should be ready for the real translations now. 😅

ludoboludo
ludoboludo approved these changes Jul 11, 2022
View reviewed changes
Copy link
Contributor

@ludoboludo ludoboludo left a comment

Choose a reason for hiding this comment

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

🚀

@andrewetchen
Copy link
Contributor

andrewetchen commented Jul 27, 2022 •
edited
Loading

Hey team,

The probot-CLA was recently deprecated and replaced with the shopify-cla action.

To successfully use the new shopify-cla status check, this PR will be closed and reopened.

Thanks and sorry for the minor inconvenience. Message me if you have any questions 😄

Please refer to the following for more information:

@andrewetchen andrewetchen reopened this Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ludoboludo ludoboludo ludoboludo approved these changes

At least 2 approving reviews are required to merge this pull request.

Assignees

@kjellr kjellr

Labels
Category: Enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kjellr @andrewetchen @ludoboludo