NIAEFEUP / nijobs-fe

[FRONTEND] A platform for companies to advertise their job opportunities to students
GNU General Public License v3.0
23 stars 3 forks source link

Feature/search url params #251

Closed Naapperas closed 1 year ago

Naapperas commented 2 years ago

Closes #220

Naapperas commented 2 years ago

I'm having a problem where, due to the use of React Contexts, the useLocation and useHistory hooks are throwing errors inside the testing environment.

I have already tried mocking the react-router-dom and react-router modules but unsuccessfully.

Here are several links all regarding the same issue (react-router hooks not working): Link 1 Link 2 Link 3 Link 4

The best solution, according to the links provided, to solve this issue is to, in every custom mount and render setup here, wrap the children to be rendered/mounted inside a Router instance: I have already tested locally using a MemoryRouter and in fact it does work. However, changing all these methods is a bit cumbersome and I cannot help but think that there might be a more efficient and elegant way to tackle this issue. Any suggestions ? @DoStini @imnotteixeira @TiagooGomess

imnotteixeira commented 2 years ago

I'm having a problem where, due to the use of React Contexts, the useLocation and useHistory hooks are throwing errors inside the testing environment.

I have already tried mocking the react-router-dom and react-router modules but unsuccessfully.

Here are several links all regarding the same issue (react-router hooks not working): Link 1 Link 2 Link 3 Link 4

The best solution, according to the links provided, to solve this issue is to, in every custom mount and render setup here, wrap the children to be rendered/mounted inside a Router instance: I have already tested locally using a MemoryRouter and in fact it does work. However, changing all these methods is a bit cumbersome and I cannot help but think that there might be a more efficient and elegant way to tackle this issue. Any suggestions ? @DoStini @imnotteixeira @TiagooGomess

This is already what we do. In every component that has a link (which implies having a Router), we wrap it in a Router. I don't think we should wrap everything in a router by default, but we should wrap every component that needs it, maybe create a Dummy utility component in your test file to automatically wrap the argument it receives in a Router similar to what we do here?

Naapperas commented 2 years ago

I am still missing the "auto-submit" feature when the search term is already present in the URL but other than that the PR is ready.

Naapperas commented 2 years ago

I came up with the solution of using a global boolean to check weather we were on the first ever render or not (relying on mounting alone, like with useEffect(..., []), does not work because apparently the component gets re-mounted after a submit, which would lead to infinite requests) but this seems like black magic. However, i have not been able to find a reasonable solution to this: any ideas ?

Naapperas commented 2 years ago

image image

This was after clicking on the search button multiple times

imnotteixeira commented 2 years ago

I came up with the solution of using a global boolean to check weather we were on the first ever render or not (relying on mounting alone, like with useEffect(..., []), does not work because apparently the component gets re-mounted after a submit, which would lead to infinite requests) but this seems like black magic. However, i have not been able to find a reasonable solution to this: any ideas ?

Yes! Welcome to React :)

That's a nice feature that hints you to not put this behavior in that component: why simulate a form submission if the user hasn't submitted anything? We can still call our own "redux-linked" search function elsewhere (I'd say on HomePage level, next to the MainView, so that we can toggle the showSearchResults variable easily. That component will not re-render and we avoid the multiple calls.

This goes in pair to what I've been suggesting for a while: to have the search system accessible by a hook, and independent from the components that use it (SearchAreas and such), but that is to be done in a separate issue

Naapperas commented 2 years ago

I came up with the solution of using a global boolean to check weather we were on the first ever render or not (relying on mounting alone, like with useEffect(..., []), does not work because apparently the component gets re-mounted after a submit, which would lead to infinite requests) but this seems like black magic. However, i have not been able to find a reasonable solution to this: any ideas ?

Yes! Welcome to React :)

That's a nice feature that hints you to not put this behavior in that component: why simulate a form submission if the user hasn't submitted anything? We can still call our own "redux-linked" search function elsewhere (I'd say on HomePage level, next to the MainView, so that we can toggle the showSearchResults variable easily. That component will not re-render and we avoid the multiple calls.

This goes in pair to what I've been suggesting for a while: to have the search system accessible by a hook, and independent from the components that use it (SearchAreas and such), but that is to be done in a separate issue

Perhaps it would be best to move the auto-submit feature to another issue (since this one is just "param binding") which ideally would be dealt with after your proposed change to the search system.

imnotteixeira commented 2 years ago

I came up with the solution of using a global boolean to check weather we were on the first ever render or not (relying on mounting alone, like with useEffect(..., []), does not work because apparently the component gets re-mounted after a submit, which would lead to infinite requests) but this seems like black magic. However, i have not been able to find a reasonable solution to this: any ideas ?

Yes! Welcome to React :)

That's a nice feature that hints you to not put this behavior in that component: why simulate a form submission if the user hasn't submitted anything? We can still call our own "redux-linked" search function elsewhere (I'd say on HomePage level, next to the MainView, so that we can toggle the showSearchResults variable easily. That component will not re-render and we avoid the multiple calls.

This goes in pair to what I've been suggesting for a while: to have the search system accessible by a hook, and independent from the components that use it (SearchAreas and such), but that is to be done in a separate issue

Perhaps it would be best to move the auto-submit feature to another issue (since this one is just "param binding") which ideally would be dealt with after your proposed change to the search system.

Honestly I don't see the advantage of having one without the other. It's not worth it to merge this without having the auto search. The params are useless otherwise

Naapperas commented 2 years ago

There is a bug where sometimes the scroll fails or the page snaps back to the top after the scroll occurs: I suspect this is because of the offer list being mounted but I do not know how to fix this, gotta look further into it

Naapperas commented 1 year ago

Change the URL copy "flow" to this:

Search URL fills the whole width (in a section above the offers list [warning: check for mobile]) There is a button that copies the URL to the clipboard (and maybe gives a visual indication about that action happening)

Naapperas commented 1 year ago

I'll have to make a video demonstrating the features developed unless someone can hook up a valid backend to the Netlify preview

BrunoRosendo commented 1 year ago

I think most of my questions are answered, just look into the small changes and I'll approve