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

chore: 🤖 remove ember radio btn, update it with hds #2556

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

DhariniJeeva
Copy link
Collaborator

@DhariniJeeva DhariniJeeva commented Nov 9, 2024

Description

https://hashicorp.atlassian.net/browse/ICU-15714
This PR removes ember-radio-btn package and updates it with HDS radio buttons

Screenshots (if appropriate)

Screenshot 2024-11-08 at 3 59 54 PM Screenshot 2024-11-08 at 3 59 49 PM Screenshot 2024-11-08 at 4 02 10 PM

How to Test

1. OIDC changes
2. Theme selection
3. Edition selection

Checklist

  • I have added before and after screenshots for UI changes
  • [ ] I have added JSON response output for API changes
  • [ ] I have added steps to reproduce and test for bug fixes in the description
  • I have commented on my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works

@DhariniJeeva DhariniJeeva requested a review from a team as a code owner November 9, 2024 00:04
Copy link

vercel bot commented Nov 9, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
boundary-ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 12, 2024 10:00pm
boundary-ui-desktop ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 12, 2024 10:00pm

@@ -67,39 +72,55 @@ module('Acceptance | auth-methods | oidc', function (hooks) {

test('can view oidc state', async function (assert) {
await visit(urls.authMethod);
await click('.rose-layout-page-actions .rose-dropdown-trigger');
await click(CHANGE_STATE_SELECTOR);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

note: I didn't use our new pattern commonSelectors since we have a separate ticket for that

{{on 'click' (fn @removeAsPrimary @model)}}
/>
<dd.Interactive {{on 'click' (fn @removeAsPrimary @model)}}>
{{t 'resources.auth-method.actions.remove-as-primary'}}
Copy link
Collaborator Author

@DhariniJeeva DhariniJeeva Nov 9, 2024

Choose a reason for hiding this comment

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

this is the new expected format by HDS, instead of text attr, we need to yield it, I updated these as I was making changes in the same page. We have a separate ticket to update it in other places

</Rose::Form>
<Hds::Form::Radio::Group
@name='edition'
class='theme-selector'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Non-blocking: I see theme-selector class is being used for the theme and edition. Could we rename it to something more generic?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated!

}

.dropdown-radio-btn {
min-width: 20rem;
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the reason to give a min width? Do we need this dropdown to have a min width by design? or its just to keep same as before?

</Rose::Form>
</dropdown.section>
</Rose::Dropdown>
<Hds::Dropdown class='change-state' data-test-change-state as |dd|>
Copy link
Collaborator

Choose a reason for hiding this comment

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

the <dd.Title @text={{t 'actions.change-state'}} /> seems to be missing

<dd.Radio
checked={{eq @model.state state}}
@value={{state}}
{{on 'click' (fn @changeState @model state)}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think for this radio component we should use {{on 'change'}} instead because a request is made when the user clicks a radio field that is already selected. However, if we use change a new request is not made when a user clicks a already selected field.

@@ -77,32 +78,6 @@ export default class ApplicationRoute extends Route {
}
}

/**
Copy link
Collaborator

Choose a reason for hiding this comment

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

great catch in removing this action!! 🙌

@@ -16,6 +31,13 @@ export default class ApplicationController extends Controller {
@service featureEdition;
@service flashMessages;

/**
* Returns available themes
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* Returns available themes
* Returns available themes
* @type {array}

small suggestion

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

Successfully merging this pull request may close these issues.

4 participants