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

Add splattibutes support and fix containerClassNames for rendered in place modals #389

Merged
merged 4 commits into from
Feb 23, 2023

Conversation

ratierd
Copy link
Contributor

@ratierd ratierd commented Feb 21, 2023

containerClassNames are not passed to the rendered in place modal.
Or rather the property is overriden by the component.

This PR allows us to have rendered in place modals take in the containerClassNames property

@ratierd
Copy link
Contributor Author

ratierd commented Feb 21, 2023

Hello @lukemelia ! Can you approve the workflow/PR ? I followed the contributing doc and script should be good.

Do I need to update the package/changelog in this PR or should I make another PR after this one to release a patch version ?

@lukemelia
Copy link
Contributor

Thanks for working on this @ratierd. I'd like to see this PR have a test so that we don't accidentally regress this improvement. As far as package version and changelog, it should not be in this PR. It is handled automatically by our release process.

@ratierd
Copy link
Contributor Author

ratierd commented Feb 23, 2023

Thanks for working on this @ratierd. I'd like to see this PR have a test so that we don't accidentally regress this improvement. As far as package version and changelog, it should not be in this PR. It is handled automatically by our release process.

Thanks for the quick reply !

I added some tests for containerClassNames given a rendered in place modal.

I also added a splattributes for the modals and some tests along with it

@lukemelia lukemelia changed the title Fix containerClassNames for rendered in place modals Add splattibutes support and fix containerClassNames for rendered in place modals Feb 23, 2023
@lukemelia
Copy link
Contributor

@ratierd this is looking good. please rebase on master to get the commit I just merged, which should resolve the CI failure.

@ratierd
Copy link
Contributor Author

ratierd commented Feb 23, 2023

@ratierd this is looking good. please rebase on master to get the commit I just merged, which should resolve the CI failure.

Done ! Thanks a lot 😄

@lukemelia lukemelia merged commit 1c23654 into yapplabs:master Feb 23, 2023
@lukemelia
Copy link
Contributor

Released as 4.1.0

@ratierd ratierd deleted the fix_in_place_dialog branch February 23, 2023 14:24
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.

None yet

2 participants