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

Feat/414 large numbers of projects on the home page are difficult to navigate #462

Conversation

myieye
Copy link
Contributor

@myieye myieye commented Dec 5, 2023

Resolves #414

My.Projects.-.Google.Chrome.2023-12-05.15-05-52.mp4

Copy link

github-actions bot commented Dec 5, 2023

UI unit Tests

1 tests   1 ✔️  0s ⏱️
1 suites  0 💤
1 files    0

Results for commit 900d91a.

♻️ This comment has been updated with latest results.

Copy link
Collaborator

@hahn-kev hahn-kev left a comment

Choose a reason for hiding this comment

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

Nice work. I like the wider grid on the home page, I didn't realize how cramped it felt before.

One thing I'd like to change is the location of the Create Project button on the home page. It used to be inline (though lower down if you have a large number of projects), but now it's hidden behind the menu. Looking at the menu I'm not really sure what I would expect to be in there without clicking it, the toggle isn't surprising, but hiding create project in the menu does seem surprising.

Could we move the create project button somewhere else? Maybe like this:
image

frontend/src/routes/(authenticated)/+page.svelte Outdated Show resolved Hide resolved
@myieye
Copy link
Contributor Author

myieye commented Dec 7, 2023

Here's the new look:
image

@myieye
Copy link
Contributor Author

myieye commented Dec 7, 2023

@hahn-kev I tried to put the mass of uninteresting changes into one commit. The big layout refactor commit doesn't result in compileable code, but it was the best I could do without having everything lumped together.

Removing the "project code not found" state from the project page was in fact relevant/part of deciding what the different page-components would be 😉.

Copy link
Collaborator

@hahn-kev hahn-kev left a comment

Choose a reason for hiding this comment

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

Testing it out I like the new design! Nice job.

But. This is what the SSR page looks like (also effects the admin page)
image
I believe this is because on the home page filteredProjects is empty until after bindings are updated from the ProjectFilter component, which reasonably only execute on the client.

Could we change filteredProjects on page to this:

let filteredProjects: ProjectItem[] = filterProjects($projects, $filters);

or, just remove filteredProjects and projects prop from ProjectFilter.svelte as it doesn't actually need the project list, it only needs the filters. Then we just use the filter function in page and AdminProjects component.

@myieye myieye force-pushed the feat/414-large-numbers-of-projects-on-the-home-page-are-difficult-to-navigate branch from 3cd7c17 to acac417 Compare December 11, 2023 10:31
@myieye
Copy link
Contributor Author

myieye commented Dec 11, 2023

@hahn-kev I removed the properties from the <ProjectFilter> as you recommended. Nice work noticing that SSR wasn't happening.

As a result, I realized that localStorage is unsatisfying, because SSR doesn't have the selected mode.
So, I started using a cookie instead, which I think is the obvious choice for configuring SSR.

It works well, but it's also a bit dissatisfying that:
a) We're sending another cookie on every request and
b) You can't access cookies in a universal-load function and the server-load in +page.server.ts gets hit everytime the user navigates to the page.
I still think it's worth it.

@myieye myieye force-pushed the feat/414-large-numbers-of-projects-on-the-home-page-are-difficult-to-navigate branch from acac417 to 41ea838 Compare December 11, 2023 10:40
@hahn-kev
Copy link
Collaborator

hahn-kev commented Dec 11, 2023

Nice, I had realize that SSR not knowing the setting but wasn't sure how difficult that would be to setup.

As for it always hitting the server side load function, you can access some cookies from JS, so I wonder if we could just use that in the load function?

Also we should be able to set a path on the cookie so it's only sent when loading that page, meaning it won't get sent with every request

@myieye
Copy link
Contributor Author

myieye commented Dec 11, 2023

@hahn-kev
Regarding reading cookies: I am reading cookies client-side as well. The issue is that +page.server.ts just seems to always get hit if it exists.
Regarding the cookie path: This cookie is for the page at our root path / and cookie paths match sub roots as well. (I.e. / matches / and /sad-face)

… event params when SSR, using browser apis client side.
Copy link
Collaborator

@hahn-kev hahn-kev left a comment

Choose a reason for hiding this comment

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

Nice work Tim!

I found a work around to eliminate the page.server file, it's a bit of a hack but it works well. Thanks to https://snippets.khromov.se/accessing-a-cookie-in-the-universal-load-function-page-js-when-using-sveltekit/ for the suggestion.

@myieye
Copy link
Contributor Author

myieye commented Dec 12, 2023

@hahn-kev I actually found that exact page, but I didn't think you'd like that 😆.
You certainly made it look tidier than I had first imagined 👍

@myieye myieye merged commit 973d890 into develop Dec 12, 2023
6 checks passed
@myieye myieye deleted the feat/414-large-numbers-of-projects-on-the-home-page-are-difficult-to-navigate branch December 12, 2023 08:00
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.

Large numbers of projects on the home page are difficult to navigate
2 participants