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

New folder button #17422

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

New folder button #17422

wants to merge 2 commits into from

Conversation

brunopagno
Copy link
Contributor

@brunopagno brunopagno commented Dec 10, 2024

Ticket

https://community.openproject.org/wp/59901

What are you trying to accomplish?

New folder button on the location picker. This allows users to organise their files better.

Screenshots

Screencast.From.2024-12-16.06-49-13.mp4

What approach did you choose and why?

Using a window.prompt because it's the easiest to implement for now. Will enable us to move forward. There are follow ups that can be done to improve UX

@brunopagno brunopagno self-assigned this Dec 10, 2024
@brunopagno brunopagno force-pushed the impl/59901-new-folder-button branch 2 times, most recently from 7144554 to c72190f Compare December 13, 2024 10:26
@brunopagno brunopagno changed the title WIP new folder button New folder button Dec 13, 2024
@brunopagno brunopagno marked this pull request as ready for review December 13, 2024 10:30
@brunopagno brunopagno requested a review from a team December 13, 2024 10:30
if (!value) { return; }

this.storageFilesResourceService.createFolder(
`${this.storage._links.self.href}/folders`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if there's a more appropriate way to build this URL

Copy link
Member

Choose a reason for hiding this comment

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

if you want to keep this URL building out of the modal, you can add it to the locals map. The createFolderHref is static, hence this could be an easy solution and you won't rebuilt it:

    const locals = {
      projectFolderHref: this.projectStorage._links.projectFolder?.href,
      projectFolderMode: this.projectStorage.projectFolderMode,
      storage,
    };

<button
type="button"
class="button spot-action-bar--action"
(click)="newFolderDialog()"
Copy link
Member

Choose a reason for hiding this comment

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

🟡 I have a naming issue here.

newFolderDialog is not what really happens here. It is instead a createFolderAndEnter().

if (!value) { return; }

this.storageFilesResourceService.createFolder(
`${this.storage._links.self.href}/folders`,
Copy link
Member

Choose a reason for hiding this comment

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

if you want to keep this URL building out of the modal, you can add it to the locals map. The createFolderHref is static, hence this could be an easy solution and you won't rebuilt it:

    const locals = {
      projectFolderHref: this.projectStorage._links.projectFolder?.href,
      projectFolderMode: this.projectStorage.projectFolderMode,
      storage,
    };

},
).subscribe((newlyCreatedDirectory) => {
this.changeLevel(newlyCreatedDirectory);
});
Copy link
Member

Choose a reason for hiding this comment

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

🟢 Yes, this implementation pretty much looks like I'd expect it.

@brunopagno brunopagno force-pushed the impl/59901-new-folder-button branch 3 times, most recently from 4704c93 to 755e181 Compare December 15, 2024 14:53
@brunopagno brunopagno requested review from Kharonus and a team December 16, 2024 05:26
@brunopagno brunopagno force-pushed the impl/59901-new-folder-button branch from b3c299b to 290974f Compare December 18, 2024 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants