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

EM-1593: Small Biz Overview Section Refactor (ESB) #112

Merged
merged 6 commits into from
Jul 10, 2017

Conversation

mrfantasticwonders
Copy link
Contributor

@mrfantasticwonders mrfantasticwonders commented Jul 6, 2017

Purpose:

The Overview section of the Small Business subscription page was designed in a way that POs could remove one of the steps through IPE, but this is not working.

I can remove some (label and paragraph) but not others (headline). Also, when I remove the label copy and then add it back, it's no longer the correct color. It should be the dark anchor blue, but switches to demon gray. (See screen shot.) In talking to David, he thinks he will need to adjust how snippets are displayed so everything is in one box per step.

This ESB PR updates the Visual Guide mixin to use flexbox's ordering system. This allows a PO to reorganize the <li> elements whichever way they want without needing to edit the guts of these blocks. This will order those blocks based on even / odd rules.

I was able to successfully wrap the entire section in a snippet and edit its source in Cortex. But there are some quirks. See Side effects here https://github.com/cbdr/employer/pull/909

This also resolves the issue with the IPE adding additional markup when copy is edited, resulting in different colored text as seen on https://hiring.careerbuilder.com/recruiting-solutions/small-business-subscription-plans-and-pricing (the header of the first section).

JIRA:
https://cb-content-enablement.atlassian.net/browse/EM-1593

Changes:

Screenshots

QA Links:
http://web.cortex.development.c66.me/legacy#/webpages (small business subscription plans and pricing EM-2)
http://web.employer-2.development.c66.me/recruiting-solutions/small-business-subscription-plans-and-pricing

How to Verify These Changes

  • Specific pages to visit

  • Steps to take

    • Go ahead and compare the actual page to https://hiring.careerbuilder.com/recruiting-solutions/small-business-subscription-plans-and-pricing . Make sure the images resize correctly and that the carousel appears under 750px. Font styling should be the same. You should also not notice the grey header seen in hiringdot on the dev server.
    • Edit the page through Cortex, in Desktop view - small business subscription plans and pricing EM-2. You should notice that the entire Overview section is editable now.
      • Try editing some copy through the IPE. Font color should not change.
      • More specifically, try editing the headers and the label used for the content step indicator.
      • View the source through IPE and try editing copy through that. Again, font color should not change.
      • View the source, and add / remove an entire <li> block. The step indicators should update. The rows should show even / odd ordering without needing to reorder the two blocks visual-guide__content (the main copy / content block) and visual-guide__image (the large image).
      • View the source, try reorganizing the <li> blocks. Again, the step indicators should update, and you should still see the even / odd ordering without needing to reorder visual-guide__content (the main copy / content block) and visual-guide__image (the large image).
  • Responsive considerations

    • The carousel should still appear and display your updated changes.
    • As mentioned before, you cannot edit this page in mobile. Doing so will cause it to break. All the edits for mobile can be done in Desktop view anyway using Source.

Relevant PRs/Dependencies:
https://github.com/cbdr/employer/pull/909

Additional Information

@@ -125,7 +118,7 @@ $visual-guide-breakpoint-small: 750px;
}

// The step must be within a <span>, not a <div>, and must contain another <span> for the actual counter
span.visual-guide__step {
div.visual-guide__step {
Copy link
Member

Choose a reason for hiding this comment

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

Typically, we don't bother with this level of specificity - it reduces the portability/re-usability of this code, requiring that any use of this class be applied to a div. I think our code can make the safe assumption that if you're using the class, you want its styles in every case for any kind of element. Also, your comment above says the step /must/ be within a span. Besides that comment needing updated to say div, is any of this truly required?

If we end up deciding to change this, there are other places in the Overview code that could incorporate this change as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@toastercup The comment and structure was a carryover from the old version of the guide. It was originally written that way due to styles that were being inherited elsewhere. However, because <div> is being used now, this no longer applies.

I went ahead and removed the comment and the specificity. This also removed the need to have the extra <div> in the container. I'm just able to use :after on .visual-guide__step and achieve the same results. Thus a developer would only need to include .visual-guide__step in the template.

Could you elaborate on the last bit?

@@ -137,8 +130,9 @@ $visual-guide-breakpoint-small: 750px;
width: $visual-guide-step-size;
color: $text-color-dark;

span:after {
div:after {
Copy link
Member

Choose a reason for hiding this comment

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

Is there a class we could specify for the after instead, rather than applying these styles to any div? I understand we're assuming the structure of this element, but I'd like to avoid that if at all possible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just went ahead and pulled this and am now basing :after off .visual-guide__step. So a developer would only need to include .visual-guide__step in the template, and nothing else.

@mrfantasticwonders
Copy link
Contributor Author

@toastercup This is ready for re-review.

@mrfantasticwonders mrfantasticwonders merged commit 0044b9e into master Jul 10, 2017
@mrfantasticwonders mrfantasticwonders deleted the topic/EM-1593-Small-Biz-Overview-Fix branch July 10, 2017 19:32
toastercup pushed a commit that referenced this pull request Jul 11, 2017
…l-Biz-Overview-Fix

EM-1593: Small Biz Overview Section Refactor (ESB)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants