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

Create HoC Live Spanish banner #37085

Merged
merged 6 commits into from
Oct 7, 2020
Merged

Create HoC Live Spanish banner #37085

merged 6 commits into from
Oct 7, 2020

Conversation

hacodeorg
Copy link
Contributor

@hacodeorg hacodeorg commented Oct 6, 2020

Task FND-1315: Create HoC Live Spanish banner on the HoC landing page (spec).

Screenshots

On desktop, the banner will look like this:

Screen Shot 2020-10-06 at 6 15 08 PM

On a smaller screen such as the iPhone 6 (375x667):

Screen Shot 2020-10-07 at 10 45 07 AM

The banner can also be resized:

Resize HoC banner - fixed

Testing story

Verify that the banner shows up for Spanish-speaking users

Verify that the banner doesn't show up for non-Spanish speaking users

Reviewer Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@hacodeorg hacodeorg changed the title Ha/hoc live spanish Create HoC Live Spanish banner Oct 6, 2020
@hacodeorg hacodeorg requested review from dju90, a team and breville October 6, 2020 11:22
@hacodeorg hacodeorg marked this pull request as ready for review October 6, 2020 11:22
@hacodeorg
Copy link
Contributor Author

This might be a basic styling technique, I don't know how to keep the height of the left and right box (image and message) the same when resizing. Any suggestions?

Resize HoC banner

@dju90
Copy link
Contributor

dju90 commented Oct 6, 2020

Can we pre-emptively add the Spanish translation to es.yml and la.yml? I can be responsible for proactively uploading the changes to en.yml to Crowdin.

Copy link
Contributor

@Hamms Hamms left a comment

Choose a reason for hiding this comment

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

The best way that I'm aware of to get sibling elements to be height-locked is to use display: table-row; on the parent element and display: table-cell; on the children. You'll also want to get rid of the current float directives:

<div class="banner-box" style="display: table-row;">
  <div class="banner-image-box" style="display: table-cell; float: none;">
    ...
  </div>
  <div class="banner-message-box" style="display: table-cell; float: none;">
    ...
  </div>
</div>

@@ -96,6 +96,52 @@ a.category-link {
margin-right: 10px;
}

.banner-box {
width: 100%;
height: fit-content;
Copy link
Contributor

@Hamms Hamms Oct 6, 2020

Choose a reason for hiding this comment

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

note that fit-content is not supported by IE, but is also I think not necessary here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

= hoc_s(:hoc_live_sign_up_title)
%div.banner-message
= hoc_s(:hoc_live_sign_up_message)
%a#signuplivebutton.ctabuttonatag{href: 'http://go.pardot.com/l/153401/2020-09-30/nwvb6j', target: '_blank'}
Copy link
Contributor

Choose a reason for hiding this comment

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

Important note: target: _blank is a security vulnerability. If we absolutely must open links in new tabs, there are some things we can do to mitigate those security risks, but in this case I don't see a need for us to do that. Situations in which opening a link in a new tab are necessary are mostly those situations in which there are interactions on the page that could be lost if the user unexpectedly navigates away; things like a page with a long form to be filled out or, say, a page with a code editor that may lose progress.

The hourofcode.com homepage is a static page, so we should just leave this anchor as normal.

For more information, see:

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 that this doesn't have to open on a new tab :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed. Thank you for the detailed explanation! It's really helpful.

@dju90
Copy link
Contributor

dju90 commented Oct 7, 2020

This might be a basic styling technique, I don't know how to keep the height of the left and right box (image and message) the same when resizing. Any suggestions?

fwiw, code studio homepage banners suffer from the same mismatched height issue when the text on the right is too long.
image

The technique they use there for smaller screen sizes is to just get rid of the image on the left when width < 993px:
image

@dju90
Copy link
Contributor

dju90 commented Oct 7, 2020

There is already a string key for "Join us" in en.yml - front_join_us_button. Should we use that instead of adding a new string key (hoc_live_join_us)? Not sure what the best practice is here :)

@hacodeorg
Copy link
Contributor Author

Can we pre-emptively add the Spanish translation to es.yml and la.yml? I can be responsible for proactively uploading the changes to en.yml to Crowdin.

^ Added!

fwiw, code studio homepage banners suffer from the same mismatched height issue when the text on the right is too long.

^ Elijah's recommendation using display: table and display: table-cell fixes the problem. Furthermore, the elements inside each cell have to be aligned using vertical-align and text-align.

There is already a string key for "Join us" in en.yml - front_join_us_button. Should we use that instead of adding a new string key (hoc_live_join_us)? Not sure what the best practice is here :)

^ The 2 "Join Us" buttons are for two different actions. The original "Join Us" button brings users to the registration form at the middle of the page. The new "Join Us" button brings user to a Pardot landing page. My instinct is we should separate them because they can change in different ways in the future.

@hacodeorg hacodeorg merged commit 9eaf5a2 into staging Oct 7, 2020
@hacodeorg hacodeorg deleted the ha/hoc-live-spanish branch October 7, 2020 18:36
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

3 participants