Added Nav-pill-scroll-to Navigation Component #1230
Conversation
Added a component that already exist in iq the nav pill scroll to navigation to the component library for general use
rpokorny
force-pushed
the
iq-nav-pills-menu-mateo-test
branch
from
502427c to
43ae87b
Compare
|
|
||
| .nx-nav-pill-menu { | ||
| // Container with indigo-90 background and 12px rounded corners | ||
| background-color: var(--nx-swatch-indigo-90); |
There was a problem hiding this comment.
It looks like only light mode is implemented. Dark mode is needed as well.
| .nx-nav-pill-menu { | ||
| // Container with indigo-90 background and 12px rounded corners | ||
| background-color: var(--nx-swatch-indigo-90); | ||
| border-radius: 12px; |
There was a problem hiding this comment.
This isn't our standard border radius. If it really should be a different radius, extract that to a named variable defined in _nx-variables.scss. But more likely you'll just want to use --nx-border-radius
| // Container with indigo-90 background and 12px rounded corners | ||
| background-color: var(--nx-swatch-indigo-90); | ||
| border-radius: 12px; | ||
| padding: 4px; // Reduced padding to achieve 41px total height |
There was a problem hiding this comment.
There's a lot of comments in here that seem like they only made sense in the context of how your code evolved in claude over time. These comments should be examined to determine which ones still have value and the rest should be removed.
| z-index: 100; | ||
| box-sizing: border-box; | ||
| // Dynamic width that hugs content but respects container bounds | ||
| width: fit-content; |
There was a problem hiding this comment.
This is very likely overkill. max-content is simpler to understand and probably does what you intend here, when used along with the max-width that you have.
| .nx-nav-pill-menu__list { | ||
| display: flex; | ||
| flex-wrap: nowrap; // Changed from wrap to nowrap for horizontal scrolling | ||
| gap: 8px; |
There was a problem hiding this comment.
| gap: 8px; | |
| gap: var(--nx-spacing-2x); |
|
|
||
|
|
||
| // Responsive adjustments | ||
| @media (max-width: 480px) { |
There was a problem hiding this comment.
We don't have specific mobile support anywhere else in RSC, so I doubt it's valuable to have it here.
|
|
||
| export { NxStatefulNavPillMenuProps }; | ||
|
|
||
| const NxStatefulNavPillMenu = forwardRef<HTMLElement, NxStatefulNavPillMenuProps>( |
There was a problem hiding this comment.
This component doesn't appear to have any state and generally appears to be redundant with NxNavPillMenu. Is there a reason that it exists?
| render(<NxNavPillMenu items={mockItems} activeItem="item1" />); | ||
|
|
||
| const activeItem = screen.getByText('Item 1'); | ||
| expect(activeItem).toHaveClass('nx-nav-pill-menu__item--active'); |
There was a problem hiding this comment.
We consider CSS classes to be implementation details, so they should not be tested
| it('applies active state correctly', () => { | ||
| render(<NxNavPillMenu items={mockItems} activeItem="item1" />); | ||
|
|
||
| const activeItem = screen.getByText('Item 1'); |
There was a problem hiding this comment.
Use getByRole with a name filter
Added a component that already exist in iq the nav pill scroll to navigation to the component library for general use