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

update: prep work for lang switcher versioning #484

Merged
merged 4 commits into from
Jun 27, 2023

Conversation

jazzyclimber
Copy link
Contributor

@jazzyclimber jazzyclimber commented Jun 27, 2023

Types of change

  • Bug fix (change which fixes an issue)
  • Enhancement (change which improves upon an existing feature)
  • New feature (change which adds new functionality)

Description

This update adds template support for the new language_switcher version. Styles overall are pretty close but have been updated a bit. DEMO PAGE

----OLD---- ----- NEW ----- -------- Description ------
2023-06-27 15 47 32 2023-06-27 15 46 08 Mobile View
2023-06-27 15 47 09 2023-06-27 15 45 44 Desktop Version

Relevant links

Example page:
GitHub issue:

Checklist

People to notify

@@ -2,6 +2,164 @@
templateType: global_partial
label: Website header
-->

{% macro add_lang_siwtcher() %}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lots of settings to get this to look similar to v0. This is because we pretty much added a million settings to the new module. These were not manually set in code but rather pulled from the page-editor using ?devMode=true. That is how I would recommend all folks pull hubl values from modules if they want to hardcode them into a template.

@jazzyclimber jazzyclimber marked this pull request as ready for review June 27, 2023 21:02
@@ -337,6 +337,19 @@
.header__language-switcher .lang_list_class:after {
content: none;
}

/* V1 lang switcher updates to keep "in line" w/ v0 mobile styles */
.header__language-switcher .hs-language-switcher__menu {
Copy link
Member

Choose a reason for hiding this comment

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

Are these just needed for the v1? If so could we toss these in a {% if get_asset_version('@hubspot/language_switcher') == 1 %} conditional?

display_mode="localized"
%}
<div class="header__language-switcher--label-current"> {{ locale_name(locale) }}</div>
{{ add_lang_siwtcher() }}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{{ add_lang_siwtcher() }}
{{ add_lang_switcher() }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hahaha thank god VS code keeps things consistent.

@@ -2,6 +2,164 @@
templateType: global_partial
label: Website header
-->

{% macro add_lang_siwtcher() %}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{% macro add_lang_siwtcher() %}
{% macro add_lang_switcher() %}

Copy link
Member

@jasonnrosa jasonnrosa left a comment

Choose a reason for hiding this comment

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

Just added a couple of minor suggestions! Beyond that, I think this looks great for a first pass. Thanks for getting it up so quickly!

Would you be able to spin up a new issue in the repo once this is all set to follow this up by exploring using theme settings as some of the module style field values (e.g. I think it could be a nice enhancement to pull in the background, text styles from theme settings > website header if that is possible to do from the global partial)?

@jazzyclimber jazzyclimber merged commit dbc0f93 into main Jun 27, 2023
1 check passed
@jazzyclimber jazzyclimber deleted the update/lang-switcher-prep branch June 27, 2023 21:19
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.

None yet

2 participants