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: update project creation #3399

Open
wants to merge 30 commits into
base: main
Choose a base branch
from

Conversation

lorenzo-cavazzi
Copy link
Member

@lorenzo-cavazzi lorenzo-cavazzi commented Nov 13, 2024

This PR updates the project and group creation pages, which are now available as modals.
The settings pages have been updated accordingly, but they will undergo further updates in another pitch.

Reference: https://www.notion.so/renku/Clean-up-Project-Group-Creation-0bee33f12b624525a100df79327eb5e7

image

image

/deploy

@RenkuBot
Copy link
Contributor

You can access the deployment of this PR at https://renku-ci-ui-3399.dev.renku.ch

@lorenzo-cavazzi lorenzo-cavazzi marked this pull request as ready for review November 22, 2024 11:01
@lorenzo-cavazzi lorenzo-cavazzi requested a review from a team as a code owner November 22, 2024 11:01
Copy link
Member

@leafty leafty left a comment

Choose a reason for hiding this comment

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

Some more comments here 👇 .

The PR is overall good, but form code needs to be improved.

client/src/features/groupsV2/new/CreateGroupButton.tsx Outdated Show resolved Hide resolved
client/src/features/groupsV2/new/GroupNew.tsx Outdated Show resolved Hide resolved
client/src/features/groupsV2/new/GroupNew.tsx Outdated Show resolved Hide resolved
client/src/features/projectsV2/fields/formField.types.ts Outdated Show resolved Hide resolved
client/src/features/projectsV2/fields/formField.types.ts Outdated Show resolved Hide resolved
client/src/features/projectsV2/fields/formField.types.ts Outdated Show resolved Hide resolved
client/src/features/groupsV2/new/GroupNew.tsx Outdated Show resolved Hide resolved
client/src/features/projectsV2/fields/SlugFormField.tsx Outdated Show resolved Hide resolved
@lorenzo-cavazzi
Copy link
Member Author

@leafty thank you for the review!

I either updated the code as suggested or dropped an inline content. Most points are solved or trivial to solve after a confirmation (E.G: Group vs GroupV2).
The one outstanding is "how to treat routes" / "where to put modals". I'm happy to talk about it offline to explain the requirements better and understand your proposed solution.

* use hashes to store the open state
* drop the projectV2 slice
client/src/features/groupsV2/new/CreateGroupButton.tsx Outdated Show resolved Hide resolved
client/src/features/projectsV2/fields/NameFormField.tsx Outdated Show resolved Hide resolved
export interface SlugFormFieldProps<T extends FieldValues>
extends GenericFormFieldProps<T> {
compact?: boolean;
countAsDirty?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

What is countAsDirty? It looks like a hack because react-hook-form is being mis-used.

Copy link
Member Author

Choose a reason for hiding this comment

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

Mis-used?
The UX has been optimized to avoid showing useless interactions; on some pages, it's easier to integrate this component seamlessly by considering other component's states. I don't think that logic should be embedded here.

client/src/features/rootV2/RootV2.tsx Outdated Show resolved Hide resolved
client/src/features/groupsV2/new/GroupNew.tsx Outdated Show resolved Hide resolved
control={control}
entityName="group"
errors={errors}
countAsDirty={dirtyFields.slug && dirtyFields.name}
Copy link
Member

Choose a reason for hiding this comment

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

Note that this is not working at the moment on the deployment: the reset button does not appear when it I type a custom slug.

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps the name wasn't filled in?
I updated the logic a little to make it more consistent. The button should show when:

  • The URL is invalid
  • The name isn't blank

and in no other case.

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't I see the reset button in this case? I filled in the name of the project and edited the slug. What if I want to reset its value now?

image

Copy link
Member

@leafty leafty left a comment

Choose a reason for hiding this comment

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

NOTE: the modals with useLocationHash are broken on the search page. Let's make an issue for this. The useSearchParams hook may be interfering with it on the search page.

@lorenzo-cavazzi
Copy link
Member Author

NOTE: the modals with useLocationHash are broken on the search page. Let's make an issue for this. The useSearchParams hook may be interfering with it on the search page.

@leafty Should we use a query parameter instead of the hash? We would have an additional useless network request, which isn't a big deal, and it should work fine on the search page.

If you prefer the hash solution (I agree it's really neat) and already have an idea of what could be the conflict, I can either open the issue or, if it's quick enough, try to fix it in this PR.

@leafty
Copy link
Member

leafty commented Nov 28, 2024

NOTE: the modals with useLocationHash are broken on the search page. Let's make an issue for this. The useSearchParams hook may be interfering with it on the search page.

@leafty Should we use a query parameter instead of the hash? We would have an additional useless network request, which isn't a big deal, and it should work fine on the search page.

If you prefer the hash solution (I agree it's really neat) and already have an idea of what could be the conflict, I can either open the issue or, if it's quick enough, try to fix it in this PR.

I highly suspect it would not work, given how the search page works. That's why fixing it later is probably fine, because creating a new project or group from the search page is not the biggest use case.

@lorenzo-cavazzi
Copy link
Member Author

@leafty Ok, sounds good 👍

I opened a new issue #3415 and added a simple workaround to avoid having broken buttons on the navbar 0bc7bfe

Copy link
Member

@leafty leafty left a comment

Choose a reason for hiding this comment

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

Should be the last small things to fix.

<FormText id={`${entityName}NameHelp`} className="input-hint">
The name you will use to refer to the {entityName}.
</FormText>
{helpText}
Copy link
Member

Choose a reason for hiding this comment

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

You should keep the <FormText> component here, otherwise the association with the input field is broken.

Suggested change
{helpText}
{helpText && <FormText id={`${entityName}-help`}>{helpText}</FormText>}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants