Skip to content

4821 part2 partner profile remove attached docs#5090

Merged
awwaiid merged 4 commits into
rubyforgood:mainfrom
danielabar:4821-part2-partner-profile-remove-attached-docs
Mar 15, 2025
Merged

4821 part2 partner profile remove attached docs#5090
awwaiid merged 4 commits into
rubyforgood:mainfrom
danielabar:4821-part2-partner-profile-remove-attached-docs

Conversation

@danielabar
Copy link
Copy Markdown
Collaborator

@danielabar danielabar commented Mar 10, 2025

Partial solution for #4821

Description

This is for part 2 of incremental improvements to editing partner profile Attached Documents section.

It adds a "Remove" button beside each attached document in the partner profile, allowing user to remove any individual document(s). Changes are persisted when they click Save Progress.

It makes use of the existing method remove_element_button in app/helpers/ui_helper.rb, along with a few styling adjustments to align the file listing and Remove action.

Update 2025-03-12: As per PR comment, added display of file names if user selects more than one. See second screenshot below.

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Automated test: spec/system/partners/profile_edit_system_spec.rb -> it "allows removal of attached documents"...

Manual test:

  1. Login as an invited partner user
  2. Navigate to Edit My Profile: http://localhost:3000/partners/profile/edit
  3. Open "Additional Documents" section
  4. Click on Choose Files and select several files (you can upload multiple at once from here)
  5. Click on "Save Progress" at bottom or top of Edit Profile form
  6. After it's saved successfully, again open "Additional Documents" section
  7. You should see all the files you uploaded earlier listed with a "Remove" button beside them.
  8. Click "Remove" for one or more of the files
  9. Click on "Save Progress" at bottom or top of Edit Profile form
  10. Again open the Attached Documents section, this time you should not see the files you removed earlier.

Screenshots

image

image

@cielf
Copy link
Copy Markdown
Collaborator

cielf commented Mar 10, 2025

Hey @danielabar -- you've got this as "resolves', but said in the issue that there would be 3 steps -- which is it?

@danielabar
Copy link
Copy Markdown
Collaborator Author

Hey @danielabar -- you've got this as "resolves', but said in the issue that there would be 3 steps -- which is it?

Sorry about that, it's a partial solution, part 2 of 3. Updated PR description.

@cielf
Copy link
Copy Markdown
Collaborator

cielf commented Mar 11, 2025

@danielabar Would it be trivial to show the names of the files chosen when you choose multiples, where it currently says "2 files" if you've selected two? We show the name if only one is chosen. (If it's not trivial, we could leave it and see if anyone complains)

Other than that, it looks pretty good!

@danielabar
Copy link
Copy Markdown
Collaborator Author

Would it be trivial to show the names of the files chosen when you choose multiples...

I'll look into it, that's a native file input. According to MDN on file input:

When the user selected multiple files, the value represents the first file in the list of files they selected. The other files can be identified using the input's HTMLInputElement.files property.

I tried selecting multiple files, then selecting that file input in browser dev tools to check it's files property, and indeed, it does have the file names available:
image

So it's a matter of how to expose this in the UI, might be possible with some JavaScript in a Stimulus controller. Will investigate further...

@danielabar
Copy link
Copy Markdown
Collaborator Author

@cielf Display of multiple file selection has been added.

@cielf
Copy link
Copy Markdown
Collaborator

cielf commented Mar 12, 2025

For future, if I ask if it's trivial, it's probably a "nice to have" so I'd rather you didn't spend too much time on it -- we can probably use your talents better elsewhere! (though it looks like you didn't have to spend a lot of time on this one)

I've run out of time for HE today -- so it'll be tomorrow or Friday before I get to this.

@danielabar
Copy link
Copy Markdown
Collaborator Author

No worries, was about an hour, and interesting to learn about the files property of the native input.

@cielf cielf requested review from awwaiid, cielf and dorner March 13, 2025 17:04
Copy link
Copy Markdown
Collaborator

@cielf cielf left a comment

Choose a reason for hiding this comment

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

LGTM - over to @awwaiid or @dorner for technical input

@cielf cielf mentioned this pull request Mar 13, 2025
}

const ul = document.createElement("ul");
ul.classList.add("list-unstyled", "mt-2");
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is a fine way to do it, but thinking to myself here I wish we didn't need the mt-2; seems like it might later be harder to maintain.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I tried removing the mt-2 and it looks fine. This has already been merged but there's still a Part 3 for this issue, so I can remove the extra margin in that PR.

Copy link
Copy Markdown
Collaborator

@awwaiid awwaiid left a comment

Choose a reason for hiding this comment

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

Looks good and good tests!

@awwaiid awwaiid merged commit 113b4c5 into rubyforgood:main Mar 15, 2025
@github-actions
Copy link
Copy Markdown
Contributor

@danielabar: Your PR 4821 part2 partner profile remove attached docs is part of today's Human Essentials production release: 2025.03.16.
Thank you very much for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants