Navigation updates, Code Security name change by zlonko · Pull Request #5395 · sourcegraph/about

zlonko · 2022-05-25T19:58:40Z

This closes #5266. It updates navigation in the header to expose use cases and reflect the renamed Code Security page. Links are also updated to support Code Security and the removal of the Product Specialist page, a.k.a. "Become One".

Specs are located in this doc.

Changelog

Restructured the desktop and mobile dropdowns for "Customers" in Header
- Renamed dropdown to "Use Cases"
- Removed links to /use-cases and /product-specialist
- Added links for the five use case pages
Flipped order of "Case Studies" and "Use Cases" in Footer
Deleted product-specialist.tsx per specs
- Added redirect from /contact/product-specialist to /demo
- Changed in-repo links to this page to demo
Updated "Fixing Vulnerabilities" to "Code Security"
- Renamed vulnerabilities.tsx to code-security.tsx
- Updated meta on code-security.tsx
- Changed in-repo links and added redirect from /use-cases/vulnerabilities to /use-cases/code-security
- Updated anchor link on /use-cases

Bug Fixes

The mobile navs on Batch Changes and Code Reuse were invisible, so those have been fixed
Code Insights was added back into the nav on this issue: Code Insights does not show in mobile nav #5391

Notes

Re: Linking the Use Case dropdown header to /use-cases
- react-bootstrap doesn't appear to support adding an href on NavDropdown: docs
- This also seems to pose a usability issue: a user could click the header wanting to expand the menu, but end up following the link (or vice versa). Also, a user may not expect this behavior, because neither the Product nor Resource dropdowns will repeat this pattern. Perhaps it is more declarative to have another nested item for "All Use Cases"? For example:

Test

Ensure linting passes.
Ensure prettier has standardized the proposed changes.
Ensure dev and production builds work.
Please check that the nav links under the "Use Case" dropdown work as expected on mobile and desktop
Navigate to /use-cases/code-security successfully
Check that anchor links on /use-cases are intact
Please test redirects for /product-specialist and /use-cases/vulnerabilities

sourcegraph-bot · 2022-05-25T23:04:25Z

Notifying subscribers in CODENOTIFY files for diff 035cc5e...74a99c8.

Notify	File(s)
@content-platform-team	netlify.toml src/components/Actions/ContactPresalesSupportAction.tsx src/components/Blog/ReleasePost.tsx src/components/Layout/Footer.tsx src/components/Layout/Header.tsx src/components/Layout/Header/DesktopNav.tsx src/components/Layout/Header/MobileNav.tsx src/components/Layout/Header/index.tsx src/components/Layout/Header/navLinks.ts src/components/Layout/index.tsx src/pages/batch-changes.tsx src/pages/case-studies/nutanix-fixed-log4j-with-sourcegraph.tsx src/pages/contact/index.tsx src/pages/contact/product-specialist.tsx src/pages/home/_UseCases.tsx src/pages/pricing.tsx src/pages/use-cases/code-reuse.tsx src/pages/use-cases/code-security.tsx src/pages/use-cases/index.tsx src/styles/components/_Header.scss

st0nebreaker

This is great, Tim, thanks for the enhancements along the way. 😄

Can you merge main back into this branch so the staging job will run? I can't test the re-directs until then, but they look like they should be good.
I don't have perms to view the doc to see all the requirements. So I can't double check requirements met yet.
Maybe a Product concern, but I feel like Case Studies being intermixed with the Use Cases drop down is a bit unorganized, right? I think Tailwind allows us to use Multi-level Dropdowns to clean this up: Tailwind-Dropdowns, Tailwind-Multilevel-Dropdowns

And a few small comments below

src/pages/batch-changes.tsx

src/pages/use-cases/code-reuse.tsx

src/components/Layout/Header.tsx

src/pages/use-cases/code-security.tsx

src/pages/use-cases/index.tsx

zlonko · 2022-05-26T21:17:06Z

@st0nebraker: The merge conflict must have happened after I requested you! I am sorry for the mix-up, it is resolved now and staging should run. I also added you/CPT to the doc and meta files.

st0nebreaker · 2022-05-26T22:46:32Z

@st0nebraker: The merge conflict must have happened after I requested you! I am sorry for the mix-up, it is resolved now and staging should run. I also added you/CPT to the doc and meta files.

All good! The staging job wasn't running in the workflow for some reason. The new commit triggered it though 👍 I see the re-directs working perfectly 😎

netlify.toml

src/components/Layout/Header.tsx

bretthayes

Looks good so far and the links work!

I'd like to make a suggestion as a refactor to make it more convenient for us to test and update these links in the future. I think we could create a nested data structure for our links and then we could add some mapping logic to render and reduce the amount of repeated markup.

Here's what I'm thinking:

const links = {
    product: [
        {
            title: 'Code Search',
            href: '/code-search'
        },
    ],
    resources: [
        {
            title: 'Code Intelligence',
            href: 'https://docs.sourcegraph.com/code_intelligence'
        },
        // ...etc.
    ],
    // ...etc.
}

Then you can use Object.keys in links to generate your IDs, react keys in your iterators, and your title attributes. This will also allow us to not have duplicate data and markup and we can render our desktop and mobile nav with the same data as the source of truth.

src/components/Layout/Header.tsx

Co-authored-by: Brett Hayes <bretthayes@users.noreply.github.com>

src/components/Layout/Header.tsx

bretthayes

Awesome improvements Tim! ✨ Feel free to either follow up or refactor a bit more in this PR 🙂

bretthayes · 2022-06-01T17:18:53Z

src/components/Layout/Header.tsx

+                                        item.href.includes('http') ? (
+                                            <Nav.Link
+                                                key={camelCase(item.title)}
+                                                href={item.href}
+                                                target="_blank"
+                                                rel="noreferrer"
+                                            >
+                                                {item.title}
+                                            </Nav.Link>
+                                        ) : (
+                                            <Nav.Link key={camelCase(item.title)} href={item.href}>
+                                                {item.title}
+                                            </Nav.Link>
+                                        )


I think we could make this a reusable rendering function since I see this used a few times. That could reduce our logic even further. You could pass a prop to render as a different element as well.

src/components/Layout/Header.tsx

bretthayes · 2022-06-01T17:54:31Z

src/components/Layout/Header.tsx

                                    >
-                                        Docs
-                                    </a>
-                                </li>
-                                <li className="nav-item" role="presentation">
-                                    <a className="nav-link" href="https://sourcegraph.com/sign-in">
                                        Sign in
                                    </a>
                                </li>
                                {!props.hideGetStartedButton && (
-                                    <li className="header__nav-item nav-item" role="presentation">
-                                        <a className="nav-link" href="https://sourcegraph.com/search">
+                                    <li className="align-items-center nav-item" role="presentation">
+                                        <a
+                                            className="nav-link"
+                                            href="https://sourcegraph.com/search"
+                                            data-button-style={buttonStyle.text}
+                                            data-button-location={buttonLocation.nav}
+                                            data-button-type="cta"
+                                        >
                                            Search code
                                        </a>
                                    </li>
                                )}
-                                <li className="header__nav-item nav-item" role="presentation">
+                                <li className="align-items-center nav-item" role="presentation">
                                    <Link href="/demo" passHref={true}>
                                        {/* eslint-disable-next-line jsx-a11y/anchor-is-valid */}
-                                        <a className="nav-link">Request a demo</a>
+                                        <a
+                                            className="nav-link"
+                                            data-button-style={buttonStyle.text}
+                                            data-button-location={buttonLocation.nav}
+                                            data-button-type="cta"
+                                        >
+                                            Request a demo
+                                        </a>
                                    </Link>
                                </li>
                                {!props.hideGetStartedButton && (
-                                    <li className="header__nav-item nav-item" role="presentation">
+                                    <li className="align-items-center nav-item" role="presentation">
                                        <Link href="/get-started" passHref={true}>
-                                            <a href="#none" className="nav-link">
+                                            {/* eslint-disable-next-line jsx-a11y/anchor-is-valid */}
+                                            <a
+                                                className="nav-link"
+                                                data-button-style={buttonStyle.text}
+                                                data-button-location={buttonLocation.nav}
+                                                data-button-type="cta"
+                                            >
                                                Get started
                                            </a>
                                        </Link>


We could even reduce the markup here as well by creating a rightNavLinks array

Ah! I agree 100%.

zlonko · 2022-06-01T21:11:43Z

@bretthayes I introduced the two components here: 59c8ef8. Also, you make good points about additional optimizations, so I created #5416 for me to loop back in when I return from PTO!

bretthayes

Looks good Tim! Looking forward to some more refactoring here ✨

st0nebreaker

Lgtm! I think some other navbar enhancements worth considering for us is (1) To limit expanded navbar items on mobile. (2) Highlight the current page in the navbar

zlonko added 5 commits May 25, 2022 14:24

rm product specialist page, redirect to demo page

4953695

Code Security name change, update links to page

e5c15de

update Header

9056928

update Footer

d378b49

update anchor links on use case landing page

91694a0

zlonko added the team/content-platform label May 25, 2022

zlonko added this to the A - Sprint 5 milestone May 25, 2022

zlonko self-assigned this May 25, 2022

zlonko added 4 commits May 25, 2022 17:09

bug, mobile nav missing on batch changes

1543d4e

path update

bb251a0

add Vulnerabilities redirect

af15739

merge conflicts

2a9a273

zlonko changed the title ~~Navigation updates~~ Navigation updates, Code Security name change May 25, 2022

zlonko marked this pull request as ready for review May 25, 2022 23:03

zlonko requested review from bretthayes and st0nebreaker May 25, 2022 23:03

add all use cases link to menu

5a31b26

st0nebreaker suggested changes May 26, 2022

View reviewed changes

merge conflicts

86a295d

zlonko added 3 commits May 27, 2022 08:22

code security title punctuation

96d7aa0

links opening in new tabs on header

fbee388

add divider in desktop nav bar

029fbb1

zlonko commented May 27, 2022

View reviewed changes

netlify.toml Show resolved Hide resolved

zlonko commented May 27, 2022

View reviewed changes

src/components/Layout/Header.tsx Outdated Show resolved Hide resolved

bretthayes suggested changes May 31, 2022

View reviewed changes

src/components/Layout/Header.tsx Outdated Show resolved Hide resolved

src/components/Layout/Header.tsx Outdated Show resolved Hide resolved

zlonko and others added 2 commits May 31, 2022 14:31

move case studies link to resources

1f67330

Update src/components/Layout/Header.tsx

1133a88

Co-authored-by: Brett Hayes <bretthayes@users.noreply.github.com>

refactor nav

ecf6b35

zlonko commented Jun 1, 2022

View reviewed changes

src/components/Layout/Header.tsx Outdated Show resolved Hide resolved

zlonko commented Jun 1, 2022

View reviewed changes

src/components/Layout/Header.tsx Outdated Show resolved Hide resolved

zlonko commented Jun 1, 2022

View reviewed changes

src/components/Layout/Header.tsx Outdated Show resolved Hide resolved

zlonko commented Jun 1, 2022

View reviewed changes

src/components/Layout/Header.tsx Outdated Show resolved Hide resolved

bretthayes reviewed Jun 1, 2022

View reviewed changes

src/components/Layout/Header.tsx Outdated Show resolved Hide resolved

bretthayes suggested changes Jun 1, 2022

View reviewed changes

zlonko added 2 commits June 1, 2022 15:55

split Header into Nav components

59c8ef8

Merge with main

97a36ee

zlonko mentioned this pull request Jun 1, 2022

Refactor navigation bar #5416

Open

bretthayes previously approved these changes Jun 1, 2022

View reviewed changes

simplify collocated file tree and add missing key

74a99c8

bretthayes dismissed their stale review via 74a99c8 June 1, 2022 22:52

bretthayes requested a review from st0nebreaker June 2, 2022 15:53

st0nebreaker approved these changes Jun 2, 2022

View reviewed changes

bretthayes merged commit 22819c5 into main Jun 2, 2022

bretthayes deleted the navigation-updates branch June 2, 2022 17:22

bretthayes mentioned this pull request Jun 2, 2022

Follow up for nav updates #5420

Merged

Conversation

zlonko commented May 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog

Bug Fixes

Notes

Test

Uh oh!

sourcegraph-bot commented May 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

st0nebreaker left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zlonko commented May 26, 2022

Uh oh!

st0nebreaker commented May 26, 2022

Uh oh!

Uh oh!

Uh oh!

bretthayes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bretthayes left a comment

Choose a reason for hiding this comment

Uh oh!

bretthayes Jun 1, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bretthayes Jun 1, 2022

Choose a reason for hiding this comment

Uh oh!

zlonko Jun 1, 2022

Choose a reason for hiding this comment

Uh oh!

zlonko commented Jun 1, 2022

Uh oh!

bretthayes left a comment

Choose a reason for hiding this comment

Uh oh!

st0nebreaker left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

zlonko commented May 25, 2022 •

edited

Loading

sourcegraph-bot commented May 25, 2022 •

edited

Loading

st0nebreaker left a comment •

edited

Loading