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

UI: HDS adoption replace <CopyButton> component #22333

Merged
merged 26 commits into from
Sep 15, 2023

Conversation

malinac02
Copy link
Contributor

@malinac02 malinac02 commented Aug 15, 2023

description updates from @hellobontempo


This PR implements the <Hds::Copy::Button> component. The component has a built in success/error states, so success flash messages have been removed as part of this adoption.
image

See below for screenshots of various implementations

Note: screenshots below say Outdated but they are not, they are marked as such when a commit is pushed to the branch after the comment was made 🙃

@malinac02 malinac02 added ui hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed labels Aug 15, 2023
@malinac02 malinac02 added this to the 1.15 milestone Aug 15, 2023
@malinac02 malinac02 force-pushed the ui/VAULT-18829/hds-adoption-replace-CopyButton branch from 2ef7656 to 86ff82c Compare August 15, 2023 01:06
@malinac02 malinac02 modified the milestones: 1.15, 1.16 Aug 17, 2023
* certificate-card.hbs: replace 1 <CopyButton> with <Hds::Copy::Button>

* scope-form.hbs: replace 1 <CopyButton> with <Hds::Copy::Button>

* fix tests caused by changing certificate-card. change hds copy button in certificate-card.hbs

* json-editor.hbs: replace 1 <CopyButton> with <Hds::Copy::Button>

* masked-input.hbs: replace 1 <CopyButton> with <Hds::Copy::Button>

* fix error with certificate-card.hbs copy button

* fix tests that deal with certificate-card.hbs

* add class to hds copy buttons to maintain similar styling to curent UI

* info-table-row.hbs: replace 2 <CopyButton> with <Hds::Copy::Button>

* undo change that should instead by merged in from main

* change tooltip copy button to white. cleanup

* add extra tet for oidc scope form. edit css class for the white icon copy button

* fix tests
@@ -34,7 +34,7 @@
<Icon @name="minus" />
{{/if}}
</div>
<div class="column is-flex-center" data-test-value-div={{@label}}>
<div class="column is-flex-center {{if @truncateValue 'is-two-thirds'}}" data-test-value-div={{@label}}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

* encrypt.hbs: replace 2 <CopyButton> with <Hds::Copy::Button>

* decrypt.hbs: replace 2 <CopyButton> with <Hds::Copy::Button>

* datakey.hbs. replace 6 <CopyButton> with <Hds::Copy::Button>

* rewrap.hbs: replace 2 <CopyButton> with <Hds::Copy::Button>

* hmac.hbs: replace 2 <CopyButton> with <Hds::Copy::Button>

* fix typo

* add copy-close class to copy & close buttons

* export.hbs: replace 2 <CopyButton> with <Hds::Copy::Button>. fix styling

* sign.hbs: replace 2 <CopyButton> with <Hds::Copy::Button>

* fix test caused by changing <pre> tag to <code> in export.hbs

* rename class

* add extra style to class needed for part 4 of copy button replacement
malinac02 and others added 2 commits September 8, 2023 09:40
* user-menu.hbs: replace 1 <CopyButton> with <Hds::Copy::Button>

* transit-form-show.hbs: replace 1 <CopyButton> with <Hds::Copy::Button>

* configure-ssh-secret.hbs: replace 1 <CopyButton> with <Hds::Copy::Button>

* tool-hash.hbs: replace 1 <CopyButton> with <Hds::Copy::Button>

* tool-random.hbs: replace 1 <CopyButton> with <Hds::Copy::Button>

* tool-rewrap.hbs: replace 1 <CopyButton> with <Hds::Copy::Button>

* tool-unwrap.hbs: replace 1 <CopyButton> with <Hds::Copy::Button>

* tool-wrap.hbs: replace 1 <CopyButton> with <Hds::Copy::Button>

* paths.hbs: replace 1 <CopyButton> with <Hds::Copy::Button>

* code-snippet.hbs: replace 1 <CopyButton> with <Hds::Copy::Button>

* cleanup css for code-snippet. add comments for getting rid of code-snippet and replacing with <Hds::Copy::Snippet

* change code-snippet copy icon to gray to match original design

* change code-snippet class

* accounts.hbs: replace 1 <CopyButton> with <Hds::Copy::Button>

* hover-copy-button.hbs: replace 1 <CopyButton> with <Hds::Copy::Button>

* add.hbs: replace 1 <CopyButton> with <Hds::Copy::Button>

* show.hbs: replace 1 <CopyButton> with <Hds::Copy::Button>

* copy-secret-dropdown.hbs: replace 1 <CopyButton> with <Hds::Copy::Button>

* change styling of 'link' copy buttons

* generate-credentials.hbs: replace 2 <CopyButton> with <Hds::Copy::Button>

* transform-show-transformation.hbs: replace 2 <CopyButton> with <Hds::Copy::Button>

* sign.hbs: replace 2 <CopyButton> with <Hds::Copy::Button>

* hide some copy buttons' icons and use original flash message

* undo cleanup of scss file so that I can put cleanup all into one PR to be more organized
hellobontempo and others added 2 commits September 13, 2023 16:34
* remove unecessary code-snippet.scssn class

* remove copy classes from masked-input.scss

* remove copy button class from text-file.scss

* uninstall ember-cli-clipboard 0.16.0 since there is no longer structure <CopyButton>

* remove copyright message from code-snippet.scss to avoid merge conflicts with main, where the file is deleted

* replace 2 classes with one

* remove unecessary class from copy button

* cleanup classes

* revert changes to avoid merge conflicts

* remove is-block class

* conditionally render private key

* add more info to comment

* remove HoverCopyButton

* add missing selector

* fix control group padding

---------

Co-authored-by: [email protected] <[email protected]>
Co-authored-by: claire bontempo <[email protected]>
Copy link
Contributor

@hellobontempo hellobontempo Sep 13, 2023

Choose a reason for hiding this comment

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

Removed this because somewhere I came across moving away from hover copy buttons because of accessibility...of course now I can't track down where I read that 🙈

@@ -47,11 +47,6 @@ export default class KeymgmtKeyEdit extends Component {
return this.args.mode === 'create';
}

@action
Copy link
Contributor

Choose a reason for hiding this comment

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

while cleaning up transit I noticed this action was unused

>
<Icon @name="clipboard-copy" aria-label="Copy" />
</CopyButton>
<div class="is-flex-center has-background-white-bis has-side-padding-s has-top-bottom-margin-negative-m">
Copy link
Contributor

Choose a reason for hiding this comment

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

Screenshot 2023-09-14 at 2 58 56 PM Screenshot 2023-09-14 at 3 00 22 PM

{{@tooltipText}}
</div>
</CopyButton>
<div class="box is-flex-center">
Copy link
Contributor

Choose a reason for hiding this comment

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

Screenshot 2023-09-14 at 3 03 55 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, copy in a tooltip seems like a serious anti-pattern but I guess that's what we had before 🙈

Copy link
Contributor

Choose a reason for hiding this comment

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

haha, my thoughts exactly! We created a polishing ticket to consider redesigning this when we initially came across it

<MaskedInput @value={{this.model.privateKey}} @name="Private key" @allowCopy={{true}} @displayOnly={{true}} />
</div>
</InfoTableRow>
{{#if this.model.privateKey}}
Copy link
Contributor

Choose a reason for hiding this comment

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

added this conditional because this row was always rendering, even when there was no private key

Copy link
Contributor

Choose a reason for hiding this comment

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

image

Copy link
Contributor

Choose a reason for hiding this comment

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

The button is slightly smaller but I think that's okay since it previously only appeared on hover

before

Screenshot 2023-09-14 at 1 48 59 PM

after

Screenshot 2023-09-14 at 2 03 33 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

This still overlaps text that's long (can make a polish task if folks thinks it's important to address later)

before

Screenshot 2023-09-14 at 2 10 14 PM

after

Screenshot 2023-09-14 at 2 10 59 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

before

Screenshot 2023-09-14 at 2 21 25 PM

after

Screenshot 2023-09-14 at 2 44 43 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

looks the same as before:
Screenshot 2023-09-14 at 2 23 31 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

updated to use CodeSnippet so they have consistent styling throughout ui

before

Screenshot 2023-09-14 at 2 50 11 PM

after

Screenshot 2023-09-14 at 2 49 59 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

consulted design and opted to remove blue copy buttons as they were redundant and anti-pattern

before / after

Screenshot 2023-09-14 at 2 52 46 PM

all of the /transit-key-action files have similar before/after views

Copy link
Contributor

Choose a reason for hiding this comment

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

Screenshot 2023-09-14 at 3 01 37 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

Went ahead and implemented the updated design:
Screenshot 2023-09-14 at 3 20 23 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

Screenshot 2023-09-14 at 3 21 01 PM

@hellobontempo hellobontempo marked this pull request as ready for review September 14, 2023 23:02
@github-actions
Copy link

Build Results:
All builds succeeded! ✅

Copy link
Contributor

@Monkeychip Monkeychip left a comment

Choose a reason for hiding this comment

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

Lots of hard work on this. Thank you both!!

Copy link
Contributor

@hashishaw hashishaw left a comment

Choose a reason for hiding this comment

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

Great (team) work!! And thank you for the screenshots 🙏

ui/lib/kv/addon/components/page/secret/paths.hbs Outdated Show resolved Hide resolved
@@ -125,7 +125,6 @@
"ember-cli": "~4.12.1",
"ember-cli-autoprefixer": "^0.8.1",
"ember-cli-babel": "^7.26.11",
"ember-cli-clipboard": "0.16.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

@hellobontempo hellobontempo enabled auto-merge (squash) September 15, 2023 20:25
@github-actions
Copy link

CI Results:
All Go tests succeeded! ✅

@hellobontempo hellobontempo merged commit 79b2f09 into main Sep 15, 2023
105 of 106 checks passed
@hellobontempo hellobontempo deleted the ui/VAULT-18829/hds-adoption-replace-CopyButton branch September 15, 2023 23:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants