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

Unification of mount state PRs #305

Open
wants to merge 37 commits into
base: develop
Choose a base branch
from
Open

Conversation

ali-abrar
Copy link
Member

No description provided.

Ross MacLeod and others added 30 commits May 20, 2017 00:09
…DomBuilder

also clean up some code around the immediate dom builder, and add some doc comments
compiles but needs testing
…s and fix the instance for ImmediateDomBuilderT

use the compiler, luke. *shakes fist at cabal*
also export MonadWidgetConstraints so it shows up in haddock
… and replace with TODO to possibly remove it
…ns, use pattern matching

to be as strict as the previous version
# Conflicts:
#	reflex-dom-core/src/Reflex/Dom/Builder/Immediate.hs
#	reflex-dom-core/src/Reflex/Dom/Main.hs
dfordivam and others added 5 commits January 16, 2019 19:46
…ents-refactor

* origin/dn-add-mount-state:
  Fix INLINE, and strictness
  fix build errors after merge
  resolve merge conflict in Immediate.hs
  address hlint complaints about extra parens
  rename MonadMountStatus to HasMountStatus
  change "action" to "widget" in MonadMountStatus documentation
  add space before MountState constructor list to be consistent
  instead of using a composition to weaken child map to get installations, use pattern matching
  switch to ragged alignment
  remove duplicate comment
  remove speculation about purpose for _immediateDomBuilderEnv_document and replace with TODO to possibly remove it
  don't use ViewPatterns for the few one-offs but instead use named field binding
  add caution about Unmounted being harder to see
  add lift instances of MonadMountStatus for ReaderT and PostBuildT
  add MonadMountStatus to MonadWidget
  remove (Reflex t, Monad m) superclass constraint from MonadMountStatus and fix the instance for ImmediateDomBuilderT
  instance MonadMountStatus for ImmediateDomBuilderT
  fix typo in doc comment
  support reporting of DOM mount state via Dynamic when using ImmediateDomBuilder
@ali-abrar
Copy link
Member Author

This contains #271 and #273 so far. #273 uses a Dynamic mount state approach that has some performance overhead. #272 provides an event interface that lets you know when something is mounted, which doesn't have the performance overhead.

The options now are:
(a) Merge #272 on top of this and switch to the Event interface, or, (b) Don't merge #272 and incur the penalty.

Regardless of which of these options we select, the next step will be to merge develop into this.

I'm going to pursue option (a) - it gets us most of the benefit without the cost.

@3noch
Copy link
Member

3noch commented Apr 2, 2019

I agree that Event-based API seems sufficient for the cases I've encountered. I can't recall if it includes "Unmounted" event? If so we can create the dynamic ourselves when needed.

…nt-state-merge

* origin/dn-getmounted-event:
  Add tests for traverseDMapWithKeyWithAdjustWithMove, comments, fixes
  Add more tests for traverseDMapWithKeyWithAdjust
  Add instances of HasMountStatus for other classes
  Add traverseDMapWithKeyWithAdjust tests
  Add tests for runWithReplace
  Add mountedEvent testcase
  Fix slowdown by inline
  Add getMounted Event
@treeowl
Copy link
Contributor

treeowl commented Jun 26, 2019

Is someone still working on this?

@ali-abrar
Copy link
Member Author

It was paused as we fixed some hydration issues. I'll have a look at the conflicts with develop and hopefully get this merged very soon.

@purefn
Copy link

purefn commented Jan 21, 2020

Any recent updates on this? I'm attempting to implement a wrapper around the Material Design Components for the web https://material.io/develop/web/ and am a bit stuck figuring out how to handle calling the clean up methods on the components that need them.

@purefn
Copy link

purefn commented Jan 22, 2020

If someone were to pick this up, what is left to do? Obviously resolve merge conflicts. If it were me I would also add an unmount event. Is there anything else that needs to be done?

@kokobd
Copy link

kokobd commented Mar 23, 2021

Are there any update on this?

@o1lo01ol1o
Copy link

o1lo01ol1o commented Oct 4, 2023

ping. It's currently difficult interface with several of the web-API methods (eg: https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollIntoView) because there's no way to check if something actually exists in the DOM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change clean up Removes stale code, improves read-ability, or small refactors enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants