-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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: Part 3 - hds adoption replace <Modal> #23415
UI: Part 3 - hds adoption replace <Modal> #23415
Conversation
box-shadow: none; | ||
border: 1px solid transparent; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy button doesn't have a @color=tertiary
option so this styling will likely remain
@@ -95,15 +95,14 @@ | |||
</div> | |||
{{/if}} | |||
<DownloadButton | |||
class="button is-ghost" | |||
@color="tertiary" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most download actions have the icon trailing, so opted to make that a default instead of another arg to pass in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed this! HDS defaults to 'leading' so keeping that functionality to align with other products
@@ -29,14 +29,14 @@ | |||
<Toolbar> | |||
<ToolbarActions> | |||
<DownloadButton | |||
class="toolbar-link" | |||
class="toolbar-button" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<button type="button" class="button download-button" {{on "click" (fn (mut this.modalOpen) true)}}> | ||
<Icon data-test-download-icon @name="download" /> | ||
</button> | ||
<Hds::Button |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@type="warning" | ||
> | ||
<section class="modal-card-body"> | ||
{{#if this.modalOpen}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -8,14 +8,14 @@ | |||
<ToolbarActions> | |||
{{#if this.model}} | |||
<DownloadButton | |||
class="toolbar-link" | |||
class="toolbar-button" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -95,15 +95,14 @@ | |||
</div> | |||
{{/if}} | |||
<DownloadButton | |||
class="button is-ghost" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Purposefully not matching css 1:1 with our old styling.
I'd prefer to make adjustments globally after we've removed the .button
class, while consulting design, so we can align with other products and rely on HDS styling as much as possible
Build Results: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just one question about the icons for download in toolbar. I feel like it should still be a download icon even if it's in the toolbar.
Download policy | ||
<Chevron @isButton={{true}} /> | ||
</DownloadButton> | ||
@icon="chevron-right" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a little odd to me that the icon here is a chevron. It seems to indicate on other toolbars that we will take you to a new page 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree! I was just keeping with the old pattern but I can update to a download :)
Download CA cert | ||
<Chevron @isButton={{true}} /> | ||
</DownloadButton> | ||
@icon="chevron-right" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here re:icon
@@ -6,7 +6,7 @@ | |||
<PkiPaginatedList @listRoute="keys.index" @list={{@keyModels}} @hasConfig={{@hasConfig}}> | |||
<:actions> | |||
{{#if @canImportKey}} | |||
<ToolbarLink @route="keys.import" @type="download" data-test-pki-key-import> | |||
<ToolbarLink @route="keys.import" @type="upload" data-test-pki-key-import> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just want to make sure this update to type upload
was intentional! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah! I noticed this was incorrect - since it's an Import
action it should be upload 😄
* UI: Part 1 - hds adoption replace <Modal> (#23363) * replace policy-form modal * replace clients/attribution modal * clients/config modal * scope form odal * remove button type * include toolbar to match other example templates * rotate credentials modal * add toolbar button class for hds buttons * transformation-edit modal * add back test selector * add route arg to button! * update link status * fix link-status tests * remove prevent default * update db tests * update tests * use page alert for hcp link status banner * fix scopy button selector * fix sidebar test * change to neutral banner * UI: Part 2 - hds adoption replace <Modal> (#23398) * upgrade HDS library (adds support for snippet containers * cleanup flight icons * replace transit key action modals * re-add deps as devDeps * remove line * address transit tests * UI: Part 3 - hds adoption replace <Modal> (#23415) * cleanup css * cleanup extra type attr * masked input download modal * use Hds::Button in download button" * fix size of modal * tiny icon fix * refactor download button to always render download icon * update tests * UI: Part 3.5 - hds adoption replace <Modal> (#23448) * replication-promote modal * replication component modals * replication add secondary modal * move update text for diff * UI: Part 4 - hds adoption replace <Modal> (#23451) * k8 configure modal * kv delete modal * ldap modals * pki modals * add trash icon * move deps * UI: Part 5 - hds adoption replace <Modal> (#23471) * replace confirmation modals --------- * UI: Part 6 - hds adoption replace <Modal> (#23484) * search select with modal * policy search select modal * replace date dropdown for client dashboard * change padding to top * update policy example args * lolllll test typo wow * update dropdown tests * shamir flow modals! * add one more container * update test selectors * UI: Final hds adoption replace <Modal> cleanup PR (#23522) * search select with modal * policy search select modal * replace date dropdown for client dashboard * change padding to top * update policy example args * lolllll test typo wow * update dropdown tests * shamir flow modals! * add one more container * update test selectors * remove wormhole and modal component * fix selectors * uninstall wormhole * remove shamir-modal-flow class * fix confirm modal test * fix pki and kv test * fix toolbar selector kv * client and download button test * fix-confirmation-modal-padding * fix replication modal tests so relevant modal opens (#23540) * more confirmation modal tests * adds changelog
The modal in
MaskedInput
includes aDownloaButton
so I opted to go ahead and implement<Hds::Button>
there instead of wrangle CSS to accommodate me 😅