Skip to content

Conversation

@kimberleyamackenzie
Copy link
Contributor

@kimberleyamackenzie kimberleyamackenzie commented Oct 13, 2025

Add a new toggle on the Marketing site that turns on "Neo mode," which opens a Neo prompt on the homepage that is deeplinked to a Neo query in the console. Note that suggested queries have been updated since below screenshots.

Screenshot 2025-10-14 at 2 47 23 PM Screenshot 2025-10-14 at 2 48 23 PM Screenshot 2025-10-14 at 4 53 04 PM

@claude
Copy link
Contributor

claude bot commented Oct 13, 2025

Pull Request Review: Neo Mode Implementation

I've reviewed PR #16247 implementing Neo Mode for the marketing site homepage. Here are my findings:

Critical Issues

1. Missing newline at end of file (layouts/index.html)

Line: 349 (end of file)
The file must end with a newline character per repository standards (AGENTS.md).

2. Accessibility issues

Lines: Throughout layouts/index.html Neo Mode implementation

  • The toggle button lacks proper ARIA labels and keyboard accessibility
  • Suggest adding: aria-label, role, and tabindex attributes
  • The Neo interface should have aria-hidden when hidden and proper focus management when shown

3. Inline event handlers

Lines: 308, 334 in layouts/index.html
Using inline onclick attributes is not best practice. Consider attaching event handlers via the script block using addEventListener.

4. Hardcoded magic number

Lines: 81, 84 in theme/src/scss/neo-mode.scss
The value 224px appears hardcoded for top position in #neo-interface. This should either be a CSS variable or have a comment explaining what it represents (likely header height).

Style & Documentation Issues

5. Inconsistent spelling

PR description
The description uses deeplinked but the correct spelling should be deep-linked (hyphenated compound adjective).

6. Missing code comments

Lines: 308-349 in layouts/index.html
The JavaScript functions lack documentation comments explaining their purpose and behavior. This would improve maintainability.

7. Generic alt text

Lines: 314, 316 in layouts/index.html
Alt text like toggle on and toggle off could be more descriptive, e.g., Toggle Neo Mode on and Toggle Neo Mode off.

Minor Issues

8. Console error handling

Lines: 335-343 in layouts/index.html
The submitNeoQuery() function doesn't validate the input or provide user feedback if the redirect fails. Consider adding basic validation or error handling.

9. CSS specificity

Lines: Throughout theme/src/scss/neo-mode.scss
Using body #neo-mode-toggle creates unnecessary specificity. Consider using classes instead for better maintainability.

10. Duplicate CSS rules

Lines: 176-197 in theme/src/scss/neo-mode.scss
The .neo-hover-icon styles are duplicated in the neo-active state. This could be refactored to reduce repetition.

Positive Notes

  • Clean visual implementation with smooth animations
  • Good use of CSS transitions and transforms
  • Properly integrates with existing homepage structure
  • The redirect URL construction is correct

Testing Recommendations

  1. Test keyboard navigation (Tab, Enter, Escape keys)
  2. Test with screen readers
  3. Test on mobile devices (the 215px fixed width may need responsive handling)
  4. Test the deep-link URL to ensure it works correctly in the console
  5. Verify that linting passes: make lint

Summary

The implementation is functionally solid but needs accessibility improvements and minor code quality fixes. The most critical fix is adding the missing newline at EOF. Address the accessibility concerns before merging to ensure the feature is usable by all users.

@kimberleyamackenzie
Copy link
Contributor Author

@vctrfrnndz Here's the first take - I have to implement responsive versions still, but this should be good to review the primary case for any early feedback!

@pulumi-bot
Copy link
Collaborator

@kimberleyamackenzie
Copy link
Contributor Author

@vctrfrnndz actually scratch that - i worked on some responsiveness too, so feel free to look/review that as well!

@pulumi-bot
Copy link
Collaborator

@pulumi-bot
Copy link
Collaborator

@vctrfrnndz
Copy link

vctrfrnndz commented Oct 14, 2025

Looking good!

Some raw thoughts:

  • Theres some empty whitespace above the takeover in neo mode, it would be good to tighten
  • Lets cap the container on the neo content to max-width 800px to force the h2 text to wrap accordingly.
  • Lets do a tighter line height on the h2.
  • Placeholder text is too subtle, would bump it to our gray-600 (#757576).
  • Padding right on the input wrapper should be lower, about 14px.
CleanShot 2025-10-14 at 15 07 31@2x

Mobile

  • Would switch sooner (same width as the top nav), to the mobile version of the badge
  • Bump the h2 text in mobile to 2rem.
  • Make the input text smaller in mobile, about 14px.
  • For the badge: Would just keep the icon on active state too in mobile

@pulumi-bot
Copy link
Collaborator

@pulumi-bot
Copy link
Collaborator

@pulumi-bot
Copy link
Collaborator

@pulumi-bot
Copy link
Collaborator

@pulumi-bot
Copy link
Collaborator

Comment on lines +256 to +265
body #main-content {
transition: opacity 0.3s ease, transform 0.3s ease;
max-width: 100vw;
overflow-x: hidden;
}

// Global body styles for Neo mode
body {
overflow-x: hidden;
max-width: 100vw;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this scoped to neo or more global? We seem to modify body in both main.scss and _marketing too. Which might be a bit confusing sometimes but is a bit of nit.

Copy link
Contributor

@foot foot left a comment

Choose a reason for hiding this comment

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

Very slick 💯 .

Did some testing on saf/ff/chrome and all seems to work great.

@pulumi-bot
Copy link
Collaborator

@pulumi-bot
Copy link
Collaborator

@pulumi-bot
Copy link
Collaborator

@pulumi-bot
Copy link
Collaborator

@pulumi-bot
Copy link
Collaborator

@pulumi-bot
Copy link
Collaborator

@kimberleyamackenzie kimberleyamackenzie merged commit ce2d2f8 into master Oct 17, 2025
8 checks passed
@kimberleyamackenzie kimberleyamackenzie deleted the km/neo-mode branch October 17, 2025 20:15
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.

8 participants