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.

Sign up for GitHub

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

Jump to bottom

[#8661] Add 'Save Profile' button #5930

Closed
wants to merge 0 commits into from
Closed

[#8661] Add 'Save Profile' button #5930

wants to merge 0 commits into from

Conversation

sevfurneaux
Copy link
Collaborator

@sevfurneaux sevfurneaux commented Jan 13, 2025 •
edited
Loading

Describe your changes
This PR adds the "Save Search Profile" button.

2025-01-14 16 59 58

Tasks

  • PR name contains story or task reference
  • Steps to recreate and test the changes
  • Documentation (docs and inline)
  • Tests (including n+1 and django_assert_num_queries where applicable)
  • Changelog

@sevfurneaux sevfurneaux self-assigned this Jan 13, 2025
Base automatically changed from pv-2025-01-load-search-profile to dev January 14, 2025 09:44
@sevfurneaux sevfurneaux force-pushed the sf-2025-add-save-profile-search-button branch 2 times, most recently from 7ab79e0 to ecfa444 Compare January 14, 2025 16:58
@sevfurneaux sevfurneaux changed the title [WIP] [#8661] Add 'Save Profile' button [#8661] Add 'Save Profile' button Jan 14, 2025
@sevfurneaux sevfurneaux force-pushed the sf-2025-add-save-profile-search-button branch 4 times, most recently from e8cafd3 to 1da11fe Compare January 14, 2025 17:32
sevfurneaux
sevfurneaux commented Jan 14, 2025
View reviewed changes
meinberlin/react/projects/ProjectsControlBar.jsx Outdated

useEffect(() => {
const handlePopState = () => {
window.location.reload()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This ensures the page is reloaded if browser back/forward.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why does it need that?

Copy link
Collaborator Author

@sevfurneaux sevfurneaux Jan 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Without the hard refresh, if a user clicks back/forward based on a history push state, we'd need to obtain the previous client side state for the previous page which is pretty complex!

Another option is we don't use push state to change the search-profile in the URL at all, so on successful save of search profile we do a hard refresh with the new search profile id, but from a user perspective it would be slower and maybe not as seamless. But yeah, less code complexity with this option.

Copy link
Collaborator

Choose a reason for hiding this comment

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

how about using replaceState instead? Or am I misunderstanding you?

Copy link
Collaborator Author

@sevfurneaux sevfurneaux Jan 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Using replaceState wouldn't cause a page refresh when using back/forward, which is what we need to force to reload the searchProfile for the page with the selected filters.

For example:

  1. On load, the page URL is projects?search-profile=1
  2. User interacts with filter again
  3. User saves search profile and window.history updates the URL to projects?search-profile=2
  4. If the user then clicks back or forward, the client side state of the url would change to the original projects?search-profile=1, but the state of the filters and the whole page would not update to reflect this.

@sevfurneaux sevfurneaux force-pushed the sf-2025-add-save-profile-search-button branch from 1da11fe to 36d5bc8 Compare January 14, 2025 17:44
vellip
vellip requested changes Jan 15, 2025
View reviewed changes
Copy link
Collaborator

@vellip vellip left a comment

Choose a reason for hiding this comment

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

Thank you for this good PR! Would you mind checking the comments I left you?

meinberlin/apps/plans/views.py Outdated Show resolved Hide resolved
meinberlin/apps/plans/views.py Outdated Show resolved Hide resolved
meinberlin/apps/plans/views.py Outdated Show resolved Hide resolved
meinberlin/assets/scss/components_user_facing/_search-profiles.scss Outdated Show resolved Hide resolved
meinberlin/assets/scss/components_user_facing/_search-profiles.scss Outdated Show resolved Hide resolved
meinberlin/react/plans/SaveSearchProfile.jsx Outdated Show resolved Hide resolved
meinberlin/react/plans/use-create-search-profile.js Outdated Show resolved Hide resolved
meinberlin/react/plans/use-create-search-profile.js Outdated
projectStatus
})

const name = generateName({ ...results, search: appliedFilters.search })
Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought the name is generated by the backend?

Copy link
Collaborator

Choose a reason for hiding this comment

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

#5928

Copy link
Collaborator Author

@sevfurneaux sevfurneaux Jan 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I thought the name is generated by the backend?

@vellip Oh, that's good to know: I had no idea. Shall I wait for that to merged? Then we can remove this functionality.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@goapunk @m4ra Do we need names generated in the frontend? Not really, since you're already doing it, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Whoops, looks like when I force pushed after merging dev it closed this PR 🤷‍♂️. I've created a new comment about name generation in the new PR: #5940 (comment)

meinberlin/react/projects/ProjectsControlBar.jsx Outdated

useEffect(() => {
const handlePopState = () => {
window.location.reload()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why does it need that?

meinberlin/react/plans/use-create-search-profile.js Outdated Show resolved Hide resolved
@sevfurneaux sevfurneaux force-pushed the sf-2025-add-save-profile-search-button branch from 79f3b00 to 4ab0601 Compare January 15, 2025 10:59
@sevfurneaux sevfurneaux force-pushed the sf-2025-add-save-profile-search-button branch from 4ab0601 to dfc79c4 Compare January 15, 2025 11:54
@sevfurneaux
Copy link
Collaborator Author

sevfurneaux commented Jan 15, 2025 •
edited
Loading

Oh weird, I have no idea why this closed due to force pushing 🤷‍♂️, I've created a new one: #5940

@sevfurneaux sevfurneaux deleted the sf-2025-add-save-profile-search-button branch January 15, 2025 11:56
@sevfurneaux sevfurneaux mentioned this pull request Jan 15, 2025
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vellip vellip vellip requested changes

@hom3mad3 hom3mad3 Awaiting requested review from hom3mad3

Assignees

@sevfurneaux sevfurneaux

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sevfurneaux @vellip