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

Refactor moving of modal elements to solve multiple issues #2238

Merged
merged 30 commits into from
Jun 27, 2022

Conversation

MadsBuchmann
Copy link
Contributor

@MadsBuchmann MadsBuchmann commented May 10, 2022

Which issue does this PR close?

This PR closes #2096
This PR closes #2116
This PR closes #1517

What is the new behavior?

I'm messing around with refactoring the modal to make it easier to grasp and solve a few bugs.

Before

A mutation observer was used for watching for changes in the embedded component. If a kirby-page-progress, kirby-page-title or kirby-modal-footer element was added, it would be moved to its desired spot.

The implementation of this was quite complex in my opinion and hard to grasp (And i have a suspiscion that i'm not the only one who believes this 👀)

Furthermore the bug from #2116 stems from the mutation observer not reacting when one of the aforementioned elements are added when they are wrapped in a component defined by the consumer.

To solve this we could either continue making it more complex (see the first suggested solution on #2116).. Or rethink how the moving of embedded components are done.

Resulting in ...

After

A new ModalElementsAdvertiser!

What we really want to know is: "which element is added/removed when?" such that we can move them out of the modal content and into the header/footer/where ever it belongs.

Instead of having the ModalWrapperComponent watch for changes, i've switched it around such that the embedded components announce themselves.

Each element that the modal should move registers itself using the method ModalElementsAdvertiser.registerElement on ngAfterViewInit.

This will in turn trigger an ModalElementsAdvertiser.[elementType].added$ observable, which the ModalWrapperComponent is subscribed to. The ModalWrapperComponent can then react accordingly.

It's more or less a simple pub/sub pattern.

Symmetrically there also exists a removed$ observable for when OnDestroy is triggered for a component.

It's a bit more verbose but i think it is easier to grasp ones head around as it is very clear what is executed when and has also made it possible to bundle relevant code some more.

Edit: new approach!

We inject the ModalWrapperComponent as a ModalElementsAdvertiser into the child elements that should be moved. They can then call a modalElementsAdvertiser.add[ElementName] and modalElementsAdvertiser.remove[elementName] method to register themselves in the wrapper component.

E.g. when a kirby-page-title element is added it calls modalElementsAdvertiser.addTitle on ngAfterviewInit and modalElementsAdvertiser.removeTitle on ngDestroy.

Does this PR introduce a breaking change?

  • Yes
  • No (hopefully)

Are there any additional context?

There's some remaining todos...

TODO:

  • The collapse title functionality has decided to not work... I should fix that.
  • ... Related to the above... Some tests are failing. Figure out why.
  • A lot of the tests are actually integration tests. We should split it up such that this is visible.

Checklist:

The following tasks should be carried out in sequence in order to follow the process of contributing correctly.

Reminders

  • Make sure you have implemented tests following the guidelines in: "The good: Test".
  • Make sure you have updated the cookbook with examples and showcases (for bug fixes, enhancements & new components).

Review

  • Do a self-review.
  • Request that the changes are code-reviewed
  • Request that the changes are UX reviewed (only necessary if your PR introduces visual changes)

When the pull request has been approved it will be automatically merged to stable via automerge.

@MadsBuchmann MadsBuchmann changed the base branch from main to release/v6.0.0 May 10, 2022 12:50
@github-actions github-actions bot temporarily deployed to pr-2116-modal-footer-version-2 May 19, 2022 11:23 Inactive
@github-actions github-actions bot temporarily deployed to pr-2116-modal-footer-version-2 May 19, 2022 11:34 Inactive
@github-actions github-actions bot temporarily deployed to pr-2116-modal-footer-version-2 May 19, 2022 11:38 Inactive
@RasmusKjeldgaard RasmusKjeldgaard requested a review from mictro May 24, 2022 14:34
@github-actions github-actions bot temporarily deployed to pr-2116-modal-footer-version-2 May 31, 2022 08:09 Inactive
@github-actions github-actions bot temporarily deployed to pr-2116-modal-footer-version-2 May 31, 2022 08:37 Inactive
@github-actions github-actions bot temporarily deployed to pr-2116-modal-footer-version-2 May 31, 2022 09:01 Inactive
@github-actions github-actions bot temporarily deployed to pr-2116-modal-footer-version-2 May 31, 2022 09:11 Inactive
@github-actions github-actions bot temporarily deployed to pr-2116-modal-footer-version-2 May 31, 2022 11:24 Inactive
@github-actions github-actions bot temporarily deployed to pr-2116-modal-footer-version-2 May 31, 2022 11:27 Inactive
@MadsBuchmann MadsBuchmann marked this pull request as ready for review June 1, 2022 07:08
@MadsBuchmann MadsBuchmann changed the title Modal footer bug (DRAFT) Refactor moving of modal elements to solve multiple issues Jun 1, 2022
Copy link
Contributor

@mictro mictro left a comment

Choose a reason for hiding this comment

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

Very nice result - consider for next show-and-tell 🥇

this.removeChild(new ElementRef(kirbyPageTitleElement));
}
}

Copy link
Contributor

@mictro mictro Jun 3, 2022

Choose a reason for hiding this comment

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

The modal-wrapper class has grown large (monolithic) - consider moving the handler code to a separate handler delegate class solely accessible by the modal-wrapper.

this.modalElementsAdvertiser.removeModalElement(this.modalElementType, this.elementRef);
}
}
}
Copy link
Contributor

@mictro mictro Jun 3, 2022

Choose a reason for hiding this comment

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

Nice interface which provides for a straight-forward and elegant distribution of responsibilities 👍

@mictro
Copy link
Contributor

mictro commented Jun 3, 2022

smooth

@mictro
Copy link
Contributor

mictro commented Jun 3, 2022

ezgif-2-5661164cfe

@github-actions github-actions bot temporarily deployed to pr-2116-modal-footer-version-2 June 10, 2022 12:03 Inactive
Copy link
Contributor

@mictro mictro left a comment

Choose a reason for hiding this comment

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

Really Nice 👍

Base automatically changed from release/v6.0.0 to develop June 15, 2022 07:49
@github-actions github-actions bot temporarily deployed to pr-2116-modal-footer-version-2 June 17, 2022 08:26 Inactive
@MadsBuchmann MadsBuchmann merged commit deec509 into develop Jun 27, 2022
@MadsBuchmann MadsBuchmann deleted the bug/2116-modal-footer-version-2 branch June 27, 2022 12:38
This was referenced Aug 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
2 participants