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

Added 'AdaptiveCardDesignerHost' Control #1238

Merged
merged 6 commits into from
Jul 11, 2022

Conversation

fabiofranzini
Copy link
Collaborator

Q A
Bug fix? [ ]
New feature? [X]
New sample? [ ]
Related issues? mentioned in #1237

What's in this Pull Request?

Implementation of the Adaptive Card Designer control as reusable react control.
To better implement this control and avoid code duplication, I had to refactor the "Adaptive Card Host" control due to share more code between the 'AdaptiveCardHost' and 'AdaptiveCardHostDesigner' controls.

I deleted the Fluent UI themes that were previously local to the 'AdaptiveCardHost' control and reused those placed in the 'common' folder, to use the latest version of the Teams theme implementations, used by the 'EnhancedThemeProvider' control.

Given the nature of the refactoring, I'm available for any clarifications.

Thanks!

@joelfmrodrigues
Copy link
Collaborator

@fabiofranzini another awesome PR 😁
Please give us some time to review as there are a lot of files

@fabiofranzini
Copy link
Collaborator Author

@joelfmrodrigues tell me if you need a call to understand all refactored code of "AdaptiveCardHost" control.
I'm aware of the change, but it was necessary to share the code between the two controls better. 🙂

@AJIXuMuK
Copy link
Collaborator

I really-really-really sorry @fabiofranzini for the long delay.
Finally able to review the PR.

@fabiofranzini
Copy link
Collaborator Author

Don't worries and thanks for your review! I'm on vacation now, I'll try to do that fixes in the next few days. Thanks again

@fabiofranzini
Copy link
Collaborator Author

@AJIXuMuK I've closed all change requests 🙂

@AJIXuMuK
Copy link
Collaborator

@AJIXuMuK I've closed all change requests 🙂

Thanks @fabiofranzini!
Could you also resolve conflicts so I could easily merge the PR?
Thanks!

@fabiofranzini
Copy link
Collaborator Author

@AJIXuMuK done! Thanks for another code review! 🙂

@AJIXuMuK AJIXuMuK merged commit 2612e46 into pnp:dev Jul 11, 2022
@AJIXuMuK AJIXuMuK added this to the 3.9.0 milestone Jul 11, 2022
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.

3 participants