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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update ButtonComponent usage to use url/method abstraction #10468

Merged
merged 3 commits into from
Apr 22, 2024

Conversation

aduth
Copy link
Member

@aduth aduth commented Apr 19, 2024

馃帿 Ticket

Refactoring after #10464, which supports LG-12716

馃洜 Summary of changes

Updates instances of ButtonComponent action to use the new url & method shorthand introduced in #10464.

Related comment: #10464 (comment)

馃摐 Testing Plan

Verify no regressions in the impacted buttons.

Comment on lines -24 to +20
action: ->(**tag_options, &block) do
button_to(second_mfa_reminder_path, **tag_options, &block)
end,
url: second_mfa_reminder_path,
method: :post,
Copy link
Member Author

Choose a reason for hiding this comment

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

One tricky thing is that button_to has an implicit method: :post, which needs to be made explicit with these updates, otherwise ButtonComponent defaults to :get. I've double-checked all of the button_to updates across this branch and #10464.

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM, now I'd expect we can remove the action: keyword entirely from ButtonComponent#initialize in this PR right?

@aduth
Copy link
Member Author

aduth commented Apr 19, 2024

LGTM, now I'd expect we can remove the action: keyword entirely from ButtonComponent#initialize in this PR right?

Probably could, yeah. I don't think there's any other usage of it. It could be useful for more advanced customization, but also fair to rip it out for YAGNI.

@zachmargolis
Copy link
Contributor

LGTM, now I'd expect we can remove the action: keyword entirely from ButtonComponent#initialize in this PR right?

Probably could, yeah. I don't think there's any other usage of it. It could be useful for more advanced customization, but also fair to rip it out for YAGNI.

+1 YAGNI

I think while the flexibility of it was well-intentioned, it's a really complicated API, if a new case comes along where we need additional behavior, we can cross that bridge when we get there

aduth added a commit that referenced this pull request Apr 19, 2024
@aduth
Copy link
Member Author

aduth commented Apr 19, 2024

LGTM, now I'd expect we can remove the action: keyword entirely from ButtonComponent#initialize in this PR right?

Probably could, yeah. I don't think there's any other usage of it. It could be useful for more advanced customization, but also fair to rip it out for YAGNI.

+1 YAGNI

I think while the flexibility of it was well-intentioned, it's a really complicated API, if a new case comes along where we need additional behavior, we can cross that bridge when we get there

馃憤 Removed support in b33a4e6.

Base automatically changed from aduth-backup-code-s-form-link to main April 22, 2024 12:46
changelog: Bug Fixes, Regenerate Backup Codes, Fix issues linking to confirm regenerating backup codes
Through subclassing and external component slot abstractions
@aduth aduth force-pushed the aduth-button-action-to-url branch from 57f3ca2 to 98a3c24 Compare April 22, 2024 12:47
@aduth aduth merged commit fce7d23 into main Apr 22, 2024
2 checks passed
@aduth aduth deleted the aduth-button-action-to-url branch April 22, 2024 17:48
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.

None yet

2 participants