Update fork UX to make it more similar to the project creation #1252
Conversation
* use the same rules to get the slug and validate the title. * remove local redux store. fix #1118
|
You can access the deployment of this PR at https://renku-ci-ui-1252.dev.renku.ch |
| const { btnClass, btnContent, error, fork, forkError, forkedTitle, forking } = props; | ||
|
|
||
| const [modalOpen, setModalOpen] = useState(false); | ||
| const toggle = () => setModalOpen(!modalOpen); |
There was a problem hiding this comment.
There seems to be a problem here. The first time I click the fork button after a page load, it flashes open, then closes. Subsequent clicks work, but the first one does not.
This does not happen in develop mode as often, but happens reliably in the production build. This may have something to do with the problems with tests as well.
There was a problem hiding this comment.
That's a good point and it may very well be the cause of the failing tests.
After some experiments, it looks like the mapStateToProps is re-triggered when we get responses from APIs with irrelevant content for the fork.
The cause is in the mapping function in one of the ancestor components, mapping the local project redux store to the global project view. This means we can't really find an elegant solution until the Project redux store is united with the global one.
A workaround could be storing the modal state in the mapper component ForkProjectMapper, but I fear we may need to store all the relevant data in the redux store to prevent losing the state.
There was a problem hiding this comment.
As discussed offline, the problem can't be solved in a very elegant way.
I've tried to move the main Modal component to the Project.present page to avoid flashing content. This is still not bulletproof (an unrelated project API may still return data when the modal is open and cause a re-draw), but it seems to work well in most cases.
I added a loader while we fetch the necessary data for the validation to prevent circumventing them (it's not to difficult when clicking fast and having a lot of projects).
| //client.runPipeline(forked.project.id); | ||
|
|
||
| // automatically redirect only when the user hasn't changed location | ||
| if (mounted && history.location.pathname === startingLocation) |
There was a problem hiding this comment.
This works nicely!
I found a minor issue with the UX here which is that if I close the popup, but stay on the same project page, then I still get redirected. I sort of expected the notification only if I was still looking at the pop-up, but I think the current behavior is fine.
There was a problem hiding this comment.
I agree the expected behavior is the one you described.
The notifications work with the route location, but we can easily check if the modal is still open and force showing the notification whenever that is not the case.
|
All the points should be fixed, even if moving the Modal to an ancestor component is not a very elegant solution. This makes the Fork components harder to re-use. Adapting them to non-modals is fairly easy if we will ever need to do that. |
|
The acceptance tests passed in 5cadb46 . I reverted the change before merging the PR |
|
Tearing down the temporary RenkuLab deplyoment for this PR. |
I have reused quite a few components from "new project".
I needed to create:
useSelectorhook)forkfunction. Even ifforkcould be part of the state component, we don't really need to store much in the redux store. Including it in the container component makes the whole logic easier to understand.I moved the redirect logic from the
clientfunctions to the react components and added notifications.We can improve the error handling if necessary -- most of the errors are now prevented by the realtime validation, so I wouldn't focus much on that unless we notice specific errors happening frequently.
/deploy
fix #1118