Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Njwe 2406/scott training results page #3169

Merged
merged 26 commits into from
Jan 21, 2025

Conversation

scwambach
Copy link
Collaborator

@scwambach scwambach commented Jan 14, 2025

resolves NJWE-2406

What I did:

  • updated the look and feel of the training results page
  • it now resembles more of how it was intended to look for the credential engine changes
  • notable difference, the filter toggle is not an overlay but rather pops in on the side as per one of mine and @mflymfly 's conversations
  • added in the new Alert if results are below 3
  • added in the param tags above the results
  • Dev review
  • Design review

@scwambach scwambach changed the base branch from nextjs to scott/basic-data-copy January 15, 2025 15:30
@scwambach scwambach marked this pull request as ready for review January 15, 2025 17:00
@scwambach scwambach changed the base branch from scott/basic-data-copy to nextjs January 15, 2025 17:03
Copy link
Collaborator

@mflymfly mflymfly left a comment

Choose a reason for hiding this comment

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

Getting the following error and unable to see the changes:

Error: fetch failed
at async getSearchData (rsc://React/Server/webpack-internal:///(rsc)/./src/app/training/search/utils/getSearchData.ts?0:13:24)
at async SearchPage (rsc://React/Server/webpack-internal:///(rsc)/./src/app/training/search/page.tsx?1:37:25)
at resolveErrorDev (webpack-internal:///(app-pages-browser)/./node_modules/next/dist/compiled/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.development.js:1792:63)
at processFullStringRow (webpack-internal:///(app-pages-browser)/./node_modules/next/dist/compiled/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.development.js:2071:17)
at processFullBinaryRow (webpack-internal:///(app-pages-browser)/./node_modules/next/dist/compiled/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.development.js:2059:7)
at progress (webpack-internal:///(app-pages-browser)/./node_modules/next/dist/compiled/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.development.js:2262:17)

@mflymfly mflymfly self-requested a review January 15, 2025 20:19
Copy link
Collaborator

@mflymfly mflymfly left a comment

Choose a reason for hiding this comment

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

All below changes captured in prototype link below

  • Is it possible for the drawer and cards to animate in / out? I can give some more instruction on that if it is possible.
  • Filters button: looks like disabled state in gray. Make it primary outline button.
  • Change filter button text "Hide Filters" when drawer is open. "Show filters" when drawer is closed.
  • Make green filter drawer Viewport Height on Desktop and allow the user to scroll within that window to see the lower filters
  • Filter drawer sticks to top of screen on scroll
  • 768>1040 - can we narrow the filer bar just a bit. I think it can just be 20rem just to give the cards a little more room to breathe. They get pretty narrow before breaking to mobile
  • Update gaps and max-width of filter drawer per screenshot
    image

Desktop Protoype

@scwambach scwambach requested a review from mflymfly January 16, 2025 19:23
Copy link
Collaborator

@mflymfly mflymfly left a comment

Choose a reason for hiding this comment

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

Just one change: can you make the whole banner clickable? currently just the caret opens the banner.
image

@scwambach scwambach requested a review from mflymfly January 21, 2025 14:32
Copy link
Collaborator

@mflymfly mflymfly left a comment

Choose a reason for hiding this comment

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

LGTM

@scwambach scwambach merged commit b7cd730 into nextjs Jan 21, 2025
1 of 2 checks passed
@scwambach scwambach deleted the NJWE-2406/scott-training-results-page branch January 21, 2025 15:45
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.

2 participants