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

DA Floodgate Copy UI #3395

Merged
merged 4 commits into from
Dec 19, 2024
Merged

Conversation

colloyd
Copy link
Contributor

@colloyd colloyd commented Dec 18, 2024

<!-- Before submitting, please review all open PRs. -->

* UI for the copy option

Resolves: [MWPW-161908](https://jira.corp.adobe.com/browse/MWPW-161908)
Copy link
Contributor

@sukamat sukamat left a comment

Choose a reason for hiding this comment

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

@colloyd Few observations:

  1. There might be a need to provide some sort of message that the user has to click 'Copy' button after the 'Find' step is completed. It will not be intuitive for a new user that the button context has changed and they have to click the button again. Alternatively, a 'Continue' or 'Start Copy' button in the 'Find' tab-step view might help as well.

  2. Making the text area uneditable after the process starts will be better as the list is technically locked in place unless they refresh the page. As the process pauses after 'Find' step and they have to click 'Copy' again, having a editable text-area gives the notion that it can be further updated.

  3. In the 'Find' tab-step, it shows 'Pages' and 'Fragments' card. It will be good to rename 'Fragments' to 'Fragments and Assets' to avoid any confusion as the list/count includes both.

Thoughts?

@colloyd
Copy link
Contributor Author

colloyd commented Dec 18, 2024

@sukamat, we can certainly move the button for copy into the tab. I thought I'd give a stab at trying to cut down on the number of buttons on screen at once. I added the "Finding..." text to the button while it's finding fragments, so there's some precedent for the text changing. I figured we'd see what users thought of this and if it's not clear, we would refine from there.

@colloyd
Copy link
Contributor Author

colloyd commented Dec 18, 2024

@sukamat Check it now. I disabled the text field when the user starts and I added messaging below the heading inside of the Find tab to indicate that they should press Copy to continue (after the find process is complete).

Copy link
Contributor

@sukamat sukamat left a comment

Choose a reason for hiding this comment

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

@colloyd

the & would be problematic as the name is converted as part of a classname elsewhere. My suggestion, given the details I mentioned in my slack to you would be to have a separate card for fragments and one for assets we would then need to setup the url object to include whether it was a fragment or an asset in addition to the other details I mentioned.

The classname conversion code in renderBadge() can be updated to exclude & with a simple regex change, right? Currently it seems to only handle names with space. Is there more to it than the regex change?

From what I see, separating fragments and assets in the response object and then handling them separately will be lot more work without any specific benefits.

@colloyd
Copy link
Contributor Author

colloyd commented Dec 18, 2024

@colloyd

the & would be problematic as the name is converted as part of a classname elsewhere. My suggestion, given the details I mentioned in my slack to you would be to have a separate card for fragments and one for assets we would then need to setup the url object to include whether it was a fragment or an asset in addition to the other details I mentioned.

The classname conversion code in renderBadge() can be updated to exclude & with a simple regex change, right? Currently it seems to only handle names with space. Is there more to it than the regex change?

From what I see, separating fragments and assets in the response object and then handling them separately will be lot more work without any specific benefits.

Done.

Copy link

codecov bot commented Dec 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (floodbox@28c529d). Learn more about missing BASE report.

Additional details and impacted files
@@             Coverage Diff             @@
##             floodbox    #3395   +/-   ##
===========================================
  Coverage            ?   98.84%           
===========================================
  Files               ?       70           
  Lines               ?     8660           
  Branches            ?        0           
===========================================
  Hits                ?     8560           
  Misses              ?      100           
  Partials            ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@colloyd colloyd requested a review from sukamat December 19, 2024 16:19
@sukamat sukamat merged commit e272c1a into adobecom:floodbox Dec 19, 2024
10 of 13 checks passed
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