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

Use the new slot API for the modal #3120

Merged
merged 1 commit into from
Jan 18, 2024
Merged

Use the new slot API for the modal #3120

merged 1 commit into from
Jan 18, 2024

Conversation

jcoyne
Copy link
Member

@jcoyne jcoyne commented Jan 17, 2024

Closes #3102 and #3106

@jrochkind
Copy link
Member

Oops, needed to actually work with ViewComponent 3.x, so a bug fix, yes?

@jcoyne
Copy link
Member Author

jcoyne commented Jan 17, 2024

@jrochkind yes, this is largely the same as #3106 but with the build passing (mostly)

@jrochkind
Copy link
Member

jrochkind commented Jan 17, 2024

Thanks! The difference between #3106 and this seems to be this one lacks additional specs.

Since the tests are being run with ViewComponent 3.x (yes?), which this func definitely didn't work with prior to this fix... the fact that we had green builds WITHOUT fixing #3102 definitely suggests we are missing some test coverage. So ideally we would add test coverage that would be red without this fix.

But if it's infeasible to provide for some reason, I guess better to have a fix that isn't covered by a test than no fix at all, I could approve anyway if it's infeasible to cover with test...

...but probably someone should at least manually confirm this really does solve #3102 and doesn't have any typos or whatever by manually exersizing func and confirming here they have?

@jcoyne jcoyne force-pushed the email-slots branch 3 times, most recently from 8e99db5 to e6d19db Compare January 17, 2024 22:37
@jcoyne
Copy link
Member Author

jcoyne commented Jan 17, 2024

@jrochkind I've added some tests here.

@jrochkind
Copy link
Member

Curious why #3106 didn't pass, where it differed, but approved!

@jcoyne jcoyne merged commit a36d8fb into main Jan 18, 2024
13 checks passed
@jcoyne jcoyne deleted the email-slots branch January 18, 2024 01:18
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.

Unsupported ViewComponent slots in SMS and Email tools
2 participants