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 support for rendering a custom wrapper around Element #13

Merged
merged 15 commits into from
Aug 21, 2023

Conversation

maheichyk
Copy link
Contributor

@maheichyk maheichyk commented Mar 21, 2023

This PR suggests to extend module API with possibility to provide a wrapper component to be rendered arounnd Element.

PR in element web element-hq/element-web#25537 that is linked.

Signed-off-by: Mikhail Aheichyk <mikhail.aheichyk@nordeck.net>
Copy link
Contributor

@weeman1337 weeman1337 left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution @maheichyk .

  • There should be another PR in React-SDK to actually display the banner. Can you link it here?
  • The banner feature should be mentioned in the README.

package.json Show resolved Hide resolved
test/lifecycles/BannerLifecycle.test.ts Outdated Show resolved Hide resolved
src/lifecycles/BannerLifecycle.ts Outdated Show resolved Hide resolved
src/lifecycles/BannerLifecycle.ts Outdated Show resolved Hide resolved
maheichyk and others added 6 commits March 22, 2023 11:39
Co-authored-by: Michael Weimann <mail@michael-weimann.eu>
Signed-off-by: Mikhail Aheichyk <mikhail.aheichyk@nordeck.net>
Signed-off-by: Mikhail Aheichyk <mikhail.aheichyk@nordeck.net>
Signed-off-by: Mikhail Aheichyk <mikhail.aheichyk@nordeck.net>
Signed-off-by: Mikhail Aheichyk <mikhail.aheichyk@nordeck.net>
@maheichyk
Copy link
Contributor Author

@weeman1337 thanks for your comments, I have provided the changes. Here is a PR in matrix-react-sdk matrix-org/matrix-react-sdk#10321 that is linked. Is it fine just to keep this link just in a comment or should I move it to the description?

Signed-off-by: Mikhail Aheichyk <mikhail.aheichyk@nordeck.net>
@weeman1337
Copy link
Contributor

@maheichyk sorry this issue got out of sight. Usually the workflow here is to re-request a review one changes are done.

image

This way it will be on my review list :)

Is it fine just to keep this link just in a comment or should I move it to the description?

For a better discoverability add it to the description please.

Mikhail Aheichyk added 2 commits May 12, 2023 08:54
Signed-off-by: Mikhail Aheichyk <mikhail.aheichyk@nordeck.net>
@Johennes
Copy link
Contributor

This blocks on element-hq/element-web#25537 (comment).

@maheichyk
Copy link
Contributor Author

@germain-gg we discussed this PR last Wednesday, it seems to be okay. Could we move this forward?

Copy link
Contributor

@germain-gg germain-gg left a comment

Choose a reason for hiding this comment

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

The banner use case makes a lot of sense, but after discussing with other team members internally we believe we would like to make this module a bit more generic.

Rather than a banner, we would like to suggest defining a wrapper.

Similarly to the banner, you should be able to define a React Component that will take on a children prop.
From an implementation point of view it would wrap the MatrixChat component, and let any consumer add a banner, a footer.

This brings the flexibility of a pseudo-iframe without the constraints.

package.json Show resolved Hide resolved
Signed-off-by: Mikhail Aheichyk <mikhail.aheichyk@nordeck.net>
@maheichyk
Copy link
Contributor Author

@germain-gg thanks a lot for the suggestion, sounds a good idea. I have added WrapperLifecycle and removed the banner related code. Please have a look.

@germain-gg germain-gg changed the title Add support for rendering a custom Banner on top of Element Add support for rendering a custom wrapper around Element Aug 18, 2023
Copy link
Contributor

@germain-gg germain-gg left a comment

Choose a reason for hiding this comment

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

Thank you for the updates, couple of changes in the test and that should be ready to be merged.

test/lifecycles/WrapperLifecycle.test.tsx Outdated Show resolved Hide resolved
test/lifecycles/WrapperLifecycle.test.tsx Outdated Show resolved Hide resolved
maheichyk and others added 2 commits August 18, 2023 17:29
Co-authored-by: Germain <germain@souquet.com>
Signed-off-by: Mikhail Aheichyk <mikhail.aheichyk@nordeck.net>
Copy link
Contributor

@germain-gg germain-gg left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you for updating everything.
That should be good to go 🎉

@germain-gg germain-gg dismissed weeman1337’s stale review August 18, 2023 14:58

All review comments have been addressed, and PR has been changed significantly since initial review

@germain-gg
Copy link
Contributor

@maheichyk could you update your branch with the base branch? It's preventing me from merging that pull request otherwise. And I do not have permissions as maintainer to push to your branch.

@maheichyk
Copy link
Contributor Author

@germain-gg it is updated, thanks for the message

@germain-gg germain-gg merged commit 7368dbf into matrix-org:main Aug 21, 2023
5 checks passed
@dhenneke dhenneke deleted the banner_lifecycle branch August 21, 2023 08:06
@maheichyk
Copy link
Contributor Author

maheichyk commented Aug 21, 2023

@germain-gg thanks for the review and merge! Could you please create a release of the module api, then I could finish: element-hq/element-web#25537

@germain-gg
Copy link
Contributor

@maheichyk It's been released as part of 2.1.0

@fnwbr
Copy link

fnwbr commented Sep 13, 2023

@maheichyk Thank you very much for your work on this! Do you by any chance have a public project that's going to make use of this new feature that you could link? I'd be curious to read up on it, as we might be interested in using this as well.

@maheichyk
Copy link
Contributor Author

@fnwbr yes, it is used in our opendesk-module that is public since some time now.

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.

5 participants