[DOCS] Fix interactions with versioning dropdown #9493

JessSaavedra · 2024-02-19T19:18:44Z

Description of PR changes above includes a link to an existing GitHub issue
PR title is prefixed with one of: [BUGFIX], [FEATURE], [DOCS], [MAINTENANCE], [CONTRIB]
Code is linted - run invoke lint (uses black + ruff)
Appropriate tests and docs have been updated

For more information about contributing, see Contribute.

After you submit your PR, keep the page open and monitor the statuses of the various checks made by our continuous integration process at the bottom of the page. Please fix any issues that come up and reach out on Slack if you need help. Thanks for contributing!

netlify · 2024-02-19T19:18:59Z

✅ Deploy Preview for niobium-lead-7998 ready!

Name	Link
🔨 Latest commit	`e1a030c`
🔍 Latest deploy log	https://app.netlify.com/sites/niobium-lead-7998/deploys/65d67037bb80ab0008f11ea0
😎 Deploy Preview	https://deploy-preview-9493.docs.greatexpectations.io
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

ghost · 2024-02-19T19:19:41Z

👇 Click on the image for a new way to code review

Legend

JessSaavedra · 2024-02-20T19:46:35Z

docs/docusaurus/src/theme/NavbarItem/DropdownNavbarItem.js

+
+export default function DropdownNavbarItemWrapper(props) {
+  return (
+    <DropdownNavbarItem {...props} href={'#'} />


This href is added to prevent dropdown to redirect to inner link when clicked (it only should make a redirection when a version inside the dropdown options is clicked)

I'm not sure I follow. What is the behavior with and without this change? And if we need this, do you mind throwing some comments in the code about this? I think this might be a source of confusion to future readers.

You can see the current behavior in https://docs.greatexpectations.io/docs/home/ by clicking the label of the dropdown (not one of the options but the label itself). It makes a weird redirect to why gx cloud page, so here I'm just overriding that behavior so that it stays on the current page. I think this it the simplest solution without having to change the actual implementation of the dropdown from docusaurus, as we're trying to keep that need as a minimum because it's not recommended by the documentation

I agree with @tyler-hoffman , what is the intent behavior if this href={"#"} I'm not familiarized with this approach.

@josectobar an href of just # means it's a url fragment. When it's not followed by an element id, it's a reference to the top of the page. So this basically hijacks the href to point to the current page, rather than the "why gx cloud" page that it otherwise defaults to. I did a tiny bit of looking to figure out why docusaurus defaults to that, and reported some intermediate findings here: https://greatexpectationslabs.slack.com/archives/C03B8DZCJ07/p1708537374187499.

I'm not able to see the message as I'm not part of the org 👀 could you paste that info here or send me an invite to join?

I'd like to see us figure out a better solution, but I'm okay with this in the short term!

JessSaavedra · 2024-02-20T19:48:16Z

docs/docusaurus/src/css/navbar.scss

@@ -86,6 +93,11 @@

 .navbar__link--active {
  border-bottom: 2px solid var(--ifm-color-primary);
+
+  &[href*="0.17"] {


This is added to prevent all three tabs (GX OSS, learn and api) from being selected at the samte in version 0.17. The source problem relies on a retrocompatibility problem due to extensive changes in docs structure from version 0.17 to 0.18 which will be solved by cutting this version out of the current project (similar to what has been done with version 0.16)

I don't think I understand. Looks like the links are still clickable, they just aren't styled like they are.

from being selected at the samte in version 0.17.
Was that supposed to say "at the same time"? Was there just something visually wrong?

I think this chunk of css might need to be fixed up or replaced with a different solution.

Yes, they should be clickable, but the problem is that with the fix they're all highlighted at the same time because of the structure of the docs for version 0.17, which is all under a big folder of docs and has no subfolders for the different sections. What you say is correct this is just a visual workaround so it doesn't look weird to habe three tabs with an underline at once, the purpose of this change is to get rid of the underline only for this particular version and we should take this out if we go with the approach of having that version on its own link, as version 0.16 has now

Note to anyone else looking at this: I'm fine with this approach because it's an improvement, and archiving 0.17 later this week will make fully resolve the issue.

tyler-hoffman

Thanks for looking at this. This is a hard problem.

I'm putting a blocker on this for a couple reasons, but if we exhaust other options, I'm open to having my mind changed.

The main concerns here for me are maintainability (specifically around logic in css) and the manual steps this would introduce when cutting new versions of docs. I don't have a great solution to any of this off the top of my head, but I'm happy to chat if you want!

tyler-hoffman · 2024-02-21T14:41:15Z

docs/docusaurus/versioned_sidebars/version-0.17-sidebars.json

@@ -1,227 +1,6 @@
 {
  "docs": [
    "intro",
-    {


Am I correct in assuming we'll have to make this same change to the versioned sidebars every time we cut a new release? I'm concerned about the maintainability of that approach.

Correct me if I'm wrong but is this change is intended to fix an incompatibility with this earlier version only, right? So going forward we shouldn't need to do this for newer versions is this correct?

No, this in particular is because we don't want to version GX cloud documentation, so the idea is to always remove it from all the versions that are not the current one (in this case i'm deleting the one in 0.17 so when you click on GX Cloud tab it always show the one from version 0.18 but we should keep removing them when releasing newer versions, so defintely is not a very maintainable solution in the long term

tyler-hoffman · 2024-02-21T14:45:16Z

docs/docusaurus/src/theme/NavbarItem/DropdownNavbarItem.js

+
+export default function DropdownNavbarItemWrapper(props) {
+  return (
+    <DropdownNavbarItem {...props} href={'#'} />


I'm not sure I follow. What is the behavior with and without this change? And if we need this, do you mind throwing some comments in the code about this? I think this might be a source of confusion to future readers.

tyler-hoffman · 2024-02-21T14:52:39Z

docs/docusaurus/src/css/navbar.scss

+  &:has(.home-section.navbar__link--active),
+  &:has(.gx-cloud-section.navbar__link--active) {
+    .navbar__items .dropdown {
+      display: none;


So if if I follow, we are adding classnames in the docusaurus config for the cloud and home section, then using the presence of children with those classnames that are active to tell determine if we should hide the dropdown.

I'm nervous about discoverability here. Could / should this type of logic be in js (But maybe with url inspection rather than classnames)? This just feels like more logic than I like to see in CSS. If we do end up needing to go with the css approach, maybe changing those classnames to something that describes their purpose (e.g. non-versioned, or something like that)?

I know the CSS solution is not the ideal, but this is mainly coming from the need we have to fix this ASAP and we're really limited by the tools that docusaurus allows us to use in terms o versioning or the components, as we're trying to avoid the need of ejecting the dropdown version component which is the ideal place to do the changes but it can bring a lot of problems when updating to newer versions of docusaurus. Maybe we can bring @joshzzheng to discussion here but yesterday we talked and doing this logic in CSS seemed like the best option in this moment

docs/docusaurus/src/css/navbar.scss

tyler-hoffman · 2024-02-21T14:58:25Z

docs/docusaurus/src/css/navbar.scss

@@ -86,6 +93,11 @@

 .navbar__link--active {
  border-bottom: 2px solid var(--ifm-color-primary);
+
+  &[href*="0.17"] {


I don't think I understand. Looks like the links are still clickable, they just aren't styled like they are.

from being selected at the samte in version 0.17.
Was that supposed to say "at the same time"? Was there just something visually wrong?

I think this chunk of css might need to be fixed up or replaced with a different solution.

tyler-hoffman · 2024-02-21T15:03:06Z

docs/docusaurus/src/css/searchBar.scss

@@ -1,15 +1,19 @@
-.custom-search-bar .DocSearch-Button {
-  background: none;
+.custom-search-bar {


This styling looks unrelated to the rest of the PR (same with the change in custom.scss). Would you mind splitting these changes out into a separate PR so this one can be focused only on the versioning concnerns?

Actually these are not needed anymore by using the approach of visibility: hidden instead of display: none, so I'm taking them out!

josectobar

I left a few comments but overall I agree with @tyler-hoffman.

josectobar · 2024-02-21T18:28:15Z

docs/docusaurus/versioned_sidebars/version-0.17-sidebars.json

@@ -1,227 +1,6 @@
 {
  "docs": [
    "intro",
-    {


Correct me if I'm wrong but is this change is intended to fix an incompatibility with this earlier version only, right? So going forward we shouldn't need to do this for newer versions is this correct?

josectobar · 2024-02-21T19:24:40Z

docs/docusaurus/src/theme/NavbarItem/DropdownNavbarItem.js

+
+export default function DropdownNavbarItemWrapper(props) {
+  return (
+    <DropdownNavbarItem {...props} href={'#'} />


I agree with @tyler-hoffman , what is the intent behavior if this href={"#"} I'm not familiarized with this approach.

This reverts commit 042ae24.

tyler-hoffman · 2024-02-22T14:18:18Z

docs/docusaurus/src/css/navbar.scss

@@ -86,6 +93,11 @@

 .navbar__link--active {
  border-bottom: 2px solid var(--ifm-color-primary);
+
+  &[href*="0.17"] {


Note to anyone else looking at this: I'm fine with this approach because it's an improvement, and archiving 0.17 later this week will make fully resolve the issue.

tyler-hoffman · 2024-02-22T14:22:45Z

docs/docusaurus/src/theme/NavbarItem/DropdownNavbarItem.js

+
+export default function DropdownNavbarItemWrapper(props) {
+  return (
+    <DropdownNavbarItem {...props} href={'#'} />


I'd like to see us figure out a better solution, but I'm okay with this in the short term!

JessSaavedra force-pushed the DSB-693_updates_versioning_dropdown branch 5 times, most recently from 2cf45bc to c784022 Compare February 20, 2024 19:45

JessSaavedra commented Feb 20, 2024

View reviewed changes

JessSaavedra added 7 commits February 20, 2024 16:48

Hide sidebar and show table of contents in get support page

4f015e2

Delete GX cloud files from version 0.17

042ae24

Add missing index files for OSS and Learn in version 0.17

5c90ebe

Hide version dropdown in home and gx cloud

69dedef

Prevent version dropdown redirect when clicked

98d8725

Fix activepath might not be present in version dropdown

9b066b8

Avoid swizzling from dropdown component

e878495

JessSaavedra force-pushed the DSB-693_updates_versioning_dropdown branch from c784022 to e878495 Compare February 20, 2024 19:51

tyler-hoffman requested changes Feb 21, 2024

View reviewed changes

JessSaavedra added 2 commits February 21, 2024 13:53

Change display none to visibility hidden in versions dropdown

d306937

Rollback search bar visual changes

7c4cac7

josectobar reviewed Feb 21, 2024

View reviewed changes

JessSaavedra force-pushed the DSB-693_updates_versioning_dropdown branch from 8768f3a to 7c4cac7 Compare February 21, 2024 21:19

JessSaavedra added 3 commits February 21, 2024 18:21

Revert "Delete GX cloud files from version 0.17"

6e2a9df

This reverts commit 042ae24.

Change classnames for non versioned sections in navbar

278159f

Rollback link config for home and GX Cloud

e1a030c

tyler-hoffman approved these changes Feb 22, 2024

View reviewed changes

JessSaavedra added this pull request to the merge queue Feb 22, 2024

Merged via the queue into develop with commit d9c7a5d Feb 22, 2024
62 checks passed

JessSaavedra deleted the DSB-693_updates_versioning_dropdown branch February 22, 2024 19:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DOCS] Fix interactions with versioning dropdown #9493

[DOCS] Fix interactions with versioning dropdown #9493

JessSaavedra commented Feb 19, 2024

netlify bot commented Feb 19, 2024 •

edited

ghost commented Feb 19, 2024 •

edited by ghost

Legend

JessSaavedra Feb 20, 2024

tyler-hoffman Feb 21, 2024

JessSaavedra Feb 21, 2024

josectobar Feb 21, 2024

tyler-hoffman Feb 21, 2024

JessSaavedra Feb 21, 2024 •

edited

tyler-hoffman Feb 22, 2024

JessSaavedra Feb 20, 2024

tyler-hoffman Feb 21, 2024

JessSaavedra Feb 21, 2024 •

edited

tyler-hoffman Feb 22, 2024

tyler-hoffman left a comment

tyler-hoffman Feb 21, 2024

josectobar Feb 21, 2024

JessSaavedra Feb 21, 2024

tyler-hoffman Feb 21, 2024

tyler-hoffman Feb 21, 2024

JessSaavedra Feb 21, 2024

tyler-hoffman Feb 21, 2024

tyler-hoffman Feb 21, 2024

JessSaavedra Feb 21, 2024

josectobar left a comment

josectobar Feb 21, 2024

josectobar Feb 21, 2024

tyler-hoffman Feb 22, 2024

tyler-hoffman Feb 22, 2024

[DOCS] Fix interactions with versioning dropdown #9493

[DOCS] Fix interactions with versioning dropdown #9493

Conversation

JessSaavedra commented Feb 19, 2024

netlify bot commented Feb 19, 2024 • edited

✅ Deploy Preview for niobium-lead-7998 ready!

ghost commented Feb 19, 2024 • edited by ghost

Legend

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JessSaavedra Feb 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JessSaavedra Feb 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyler-hoffman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josectobar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netlify bot commented Feb 19, 2024 •

edited

ghost commented Feb 19, 2024 •

edited by ghost

JessSaavedra Feb 21, 2024 •

edited

JessSaavedra Feb 21, 2024 •

edited