hackerspace-ntnu / website-next

Website for the Hackerspace NTNU student organization.
3 stars 0 forks source link

Add storage shopping cart, loan form and various improvements #33

Closed ZeroWave022 closed 3 weeks ago

ZeroWave022 commented 1 month ago

This PR improves the storage page by adding the following features:

michaelbrusegard commented 1 month ago

Remember to move stuff to layout.tsx, and use route groups as discussed. Also create loading.tsx for all the pages

ZeroWave022 commented 1 month ago

Resolved almost all conversations by applying fixes, but please see the comment about skeletons.

Please make sure to review the route group solution in 34e9820978a751dd125147a99cd739b16d5a027 - this required changing the location of search params, but otherwise, it went quite smoothly.

Let me know if you've got any more feedback!

michaelbrusegard commented 1 month ago

The check fails because you dont have updated dependencies. You need to run bun install and commit the lockfile, I can do it

michaelbrusegard commented 1 month ago

It does not get to run the lighthouse checks because it fails the build, you can check why it fails the build in the GitHub action or you can try building it locally using bun run build and then bun start. I want to merge it too, but can not merge when it fails building

michaelbrusegard commented 1 month ago

We should implement this so search params are cleared on default instead of setting the default value to an empty string which does not represent a value.

Edit: You would just add a snippet like this .withOptions({ shallow: false, clearOnDefault: true }) and set a default value to the actual default value and not an empty string. See here: https://nuqs.47ng.com/docs/options#clear-on-default

Edit2: The SortSelector and CategorySelector should be combined to a single component where you use useQueryStates instead to keep track of both of them. See here: https://nuqs.47ng.com/docs/batching#usequerystates

michaelbrusegard commented 1 month ago

The search bar component should include the icon inside the input, this can be achieved by setting the wrapper div to position relative and absolutely position the search icon in the input. then add some padding on the left side of the input.

michaelbrusegard commented 1 month ago

The title text is cutoff at the bottom for the items see the y in the screenshot

Screenshot 2024-09-23 at 01 30 56
michaelbrusegard commented 1 month ago

The scaling transition on the images scales it outside of its container. An easy way to see this is to look at the corners who stop being rounded when it scales

michaelbrusegard commented 1 month ago

Also most of the lighthouse warnings have to be addressed. If some of them are unnecessary we should remove them, but most of them look appropriate

michaelbrusegard commented 1 month ago

The skeletons do not match what is loaded. Also the main skeleton can be simplified by just showing the cards and not the content inside the cards. The skeleton for the pagination is missing also we can use the same paginationskeleton as the one from news

michaelbrusegard commented 1 month ago

The shopping-cart page should have a layout. There are a lot of things that can be loaded immediately and does not need to be in the suspense

michaelbrusegard commented 1 month ago

Also we should have the ability to select how many items to borrow of each on the shopping cart page

ZeroWave022 commented 1 month ago

The default value for the sorting is now popularity instead of an empty string. The default value for the category stays as the empty string, as that represents any category.

The CategorySelector and SortSelector components have not been combined into one, as it's not guaranteed we will not use them separately in the future. It's reasonable that they manage their own state.

Thanks for improving the search bar!

Line height of item names in the cards has been fixed.

Images in item cards scale properly.

The skeleton on /storage has been improved. The skeleton on /storage/shopping-cart is difficult to improve, because it's impossible for us to know how many items we have, or if we have any items in the cart at all. Therefore I'll just leave it to be a few rows of items as default.

Lighthouse fixes applied:

Lighthouse is still complaining, but everything seems to be fine (at least better than before).

There is currently (as far as I know) no easy way to give the shopping cart page a good layout. The only things that can be preloaded are the title and the buttons. However, the page is set up like this:

Title Items table Action buttons Loan form

As far as I know, you can't "split" children in the layout in a way that would make it possible to create a layout for this, assuming only the title and action buttons can be preloaded.

Added the ability to select how many items you'd like to loan!

michaelbrusegard commented 1 month ago

Lighthouse stuff We should get a more detailed report so I think something is wrong with the configuration. It should link to a webpage with more details. I will take a look at this tomorrow. The fixes below I think are fixable

Extras (Should probably be new PRs, but can be nice to discuss):

Suspense

michaelbrusegard commented 3 weeks ago

Can we merge now?

michaelbrusegard commented 3 weeks ago
Screenshot 2024-09-28 at 04 36 34

On iphone se2 the back to storage page button comes too close to the title. Either we have to make the title right aligned or smaller

ZeroWave022 commented 3 weeks ago

The issue described in the last comment is now fixed by making the title smaller om smaller screens.

Lighthouse was complaining about layout shifts on the storage page. When this was investigated locally, it pointed at the Hackerspace logo. Please see the before and after the fix for layout shifts.

Also, the lighthouse warning will be ignored when #55 goes through. The unused javascript warning is triggered by react-daypicker for some reason, even though isn't not used on the storage page.

All in all, seems ready to be merged

michaelbrusegard commented 3 weeks ago

Should we merge this now? Maybe we merge with dev to get the lighthouse changes and make sure it approves the changes (if not we add them to the ignore list)

ZeroWave022 commented 3 weeks ago

Yeah, merging