New Hero/Introduction section design: Remove Dark Hero from symbol pages #10

hqhhuang · 2022-03-11T00:10:30Z

Bug/issue #, if applicable: 89970711, 89691884

Summary

With the new Hero/Introduction section design, we are removing the dark hero design from symbol pages. This means that only Articles, Sample Code, API Collections, Top-level technology pages should render the dark Hero background.

This PR also fixes the extra gray divider line that was between the Hero and body content.

Testing

Steps:

Run this branch
Assert that only Articles, Sample Code, API Collections, Top-level technology pages have dark hero design.

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

Added tests
Ran npm test, and it succeeded
Updated documentation if necessary - NA

Remove Hero Image from symbol pages

…move-hero-from-symbol

Fix title color with hero removed and fix extra border

fix tests

…o remove-hero-from-symbol

fix spacing between hero and body

mportiz08 · 2022-03-11T01:07:17Z

src/components/DocumentationTopic.vue

  <div class="doc-topic">
    <main class="main" id="main" role="main" tabindex="0">
-      <DocumentationHero :type="symbolKind || role">
+      <DocumentationHero :type="symbolKind || role" :enableHero="enableHero">


This isn't a huge deal, but we should consider renaming this to better convey what it does—this flag simply adds/removes the special dark background for the hero as opposed to actually enabling/disabling it.

mportiz08 · 2022-03-11T01:12:53Z

src/components/DocumentationTopic.vue

+      <DocumentationHero :type="symbolKind || role" :enableHero="enableHero">
        <slot name="above-title" />
-        <Title :eyebrow="roleHeading">{{ title }}</Title>
+        <Title :eyebrow="roleHeading" :style=heroTitleStyle>{{ title }}</Title>


I don't think manually updating the style attribute is necessary here and probably not preferred. I'm actually a little surprised that the SASS functions you're using would actually work in that manner, but maybe they do.

As an alternative, I would suggest maybe just applying a class to the hero element based on the boolean flag you already have. Then, you can just update the CSS for components like the hero as normal, and they would have rules like so:

.documentation-hero { &.enhanced-background { --title-color: light-color(fill); --border-width: 0; /* etc */ } }

I got a little further in the PR, and it seems like you already have a class like this that you can already use to do this :)

I used this method for styling the Title component, but not sure if I can or should do the same for the border width. See: #10 (comment)

mportiz08 · 2022-03-11T01:16:49Z

src/components/DocumentationTopic/DocumentationHero.vue

 $doc-hero-icon-opacity: 1 !default;
 $doc-hero-icon-color: dark-color(fill-secondary) !default;

+.documentation-hero-disabled {


It looks like these are mostly duplicated from the documentation-hero class with minor adjustments. Instead of toggling between the documentation-hero and documentation-hero-disabled classes, it might be easier to have the documentation-hero class always there and only add a modifier class for when the special dark background is enabled so you don't have to duplicate as much styling—you only need to override a few small things with the modifier class.

mportiz08 · 2022-03-11T01:17:43Z

src/components/DocumentationTopic/Title.vue

 .eyebrow {
  @include font-styles(eyebrow-reduced);
-  color: light-color(fill-gray-tertiary);
+  color: var(--hero-eyebrow);


Minor thing but maybe --hero-eyebrow-color would be better indicate what the property is used for?

mportiz08 · 2022-03-11T01:19:24Z

src/styles/core/_breakpoints.scss

  // Center the content using auto left/right margins
  margin-left: auto;
  margin-right: auto;
+  margin-top: 0;


Is this strictly necessary? This mixin may be used in a lot of places where this kind of change would be difficult to test. I wonder if there might be a better way of doing this in a more scoped place where this style was needed (same for the other margin change below)

I did a bit of digging - it seems like the spacing bug this is meant to fix was only happening when the navigator is turned on. (There's extra spacing between the end of hero section and the gray border)

So I believe it is necessary to fix it in the mixin that specifically applies to the situation where the navigator is open.
@dobromir-hristov what's your thought on this?

Edit: Removed this change from the PR. Will open a separate PR to fix this in the body content style change branch.

dobromir-hristov

Marcus gave you already very good input, I will just add on top of that.

dobromir-hristov · 2022-03-11T06:13:15Z

src/components/DocumentationTopic.vue

    hideSummary: () => getSetting(['features', 'docs', 'summary', 'hide'], false),
+    enableHero: ({ symbolKind }) => (symbolKind ? (symbolKind === 'module') : true),
+    heroTitleStyle({ enableHero }) {
+      return enableHero ? {


Generally I would suggest we apply such css vars with JS logic, only in cases where we need a dynamic value from the JS env itself.

In this case it might make sense to just toggle a class on/off and apply these in pure the SCSS code.
Also light-color is a scss function, it should not work here.

src/components/DocumentationTopic/DocumentationHero.vue

dobromir-hristov · 2022-03-11T06:17:08Z

src/components/DocumentationTopic.vue

    ),
    shouldShowLanguageSwitcher: ({ objcPath, swiftPath }) => objcPath && swiftPath,
    hideSummary: () => getSetting(['features', 'docs', 'summary', 'hide'], false),
+    enableHero: ({ symbolKind }) => (symbolKind ? (symbolKind === 'module') : true),


I cant verify from a data perspective if this is correct. @mportiz08 wdyt?

This is meant for top level technologies page that has a symbol kind (module). Maybe Marcus should confirm if this is the correct logic for displaying dark hero background(enable) for the 4 types of pages we are interested in.

dobromir-hristov · 2022-03-11T06:19:45Z

src/components/DocumentationTopic.vue

+    },
+    borderWidth({ enableHero }) {
+      return enableHero ? {
+        '--border-width': '0px',


I honestly dont know if its better to have a class to turn on/off the separator or a css var like this.

The reason why I chose to use a CSS var like this is because the border is applied in the primary content component. Whether or not the hero should be "enabled" (or rather have the darker background) is not available in that component. So I would need to pass it in a prop.
I'm also not sure which way is better. Maybe Marcus can comment on this as well?

The only change this component needs to make (for removing dark background hero from symbol pages) is this border-width change. Versus in the Documentation Hero component, there is more logic so that adding this boolean prop makes sense.

use modifier

fix title styling

fix test after var name change

remove margin top edit

SamLanier

Design looks good!

Add Language Toggle to Nav Bar

hqhhuang added 5 commits March 10, 2022 00:53

Remove Hero Image from symbol pages

ad47a40

Remove Hero Image from symbol pages

Merge remote-tracking branch 'origin/left-align-hero-content' into re…

61355d7

…move-hero-from-symbol

Fix title color with hero removed

5536472

Fix title color with hero removed and fix extra border

fix tests

70e7dbf

fix tests

Merge remote-tracking branch 'dobri/dhristov/add-sidenav-hero-v2' int…

3ca8e2c

…o remove-hero-from-symbol

hqhhuang requested review from dobromir-hristov, marinaaisa and mportiz08 March 11, 2022 00:10

fix spacing between hero and body

6ca46e2

fix spacing between hero and body

mportiz08 reviewed Mar 11, 2022

View reviewed changes

dobromir-hristov reviewed Mar 11, 2022

View reviewed changes

hqhhuang added 3 commits March 11, 2022 01:55

use modifier

5050cad

use modifier

fix title styling

cd223b1

fix title styling

fix test after var name change

6be6a85

fix test after var name change

hqhhuang requested review from SamLanier, dobromir-hristov and mportiz08 March 11, 2022 10:44

remove margin top edit

4aa1f09

remove margin top edit

SamLanier approved these changes Mar 11, 2022

View reviewed changes

hqhhuang and others added 6 commits March 11, 2022 10:47

Merge remote-tracking branch 'apple/main' into remove-hero-from-symbol

e0ac31a

Merge branch 'dhristov/add-sidenav-hero-v2' into remove-hero-from-symbol

c3ec08b

Refactor enabling/disabling of primary content border.

2e7cce7

Refactor header colors.

e9dc5c9

Disable before/after pseudo elements properly.

7fdee7f

Remove unecessary change to mixin.

fcf7b70

mportiz08 approved these changes Mar 11, 2022

View reviewed changes

mportiz08 merged commit 4e3a9b1 into dobromir-hristov:dhristov/add-sidenav-hero-v2 Mar 11, 2022

dobromir-hristov pushed a commit that referenced this pull request Mar 31, 2022

Merge pull request #10 from hqhhuang/language-switcher-in-hero

24c5399

Add Language Toggle to Nav Bar

New Hero/Introduction section design: Remove Dark Hero from symbol pages #10

New Hero/Introduction section design: Remove Dark Hero from symbol pages #10

Uh oh!

Conversation

hqhhuang commented Mar 11, 2022

Summary

Testing

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dobromir-hristov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hqhhuang Mar 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SamLanier left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hqhhuang Mar 11, 2022 •

edited

Loading