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

fix(universal): correct fxLayout rendering, update LICENSE #466

Closed
wants to merge 1 commit into from

Conversation

CaerusKaru
Copy link
Member

@CaerusKaru CaerusKaru commented Oct 22, 2017

@ThomasBurleson
Copy link
Contributor

@CaerusKaru - has this PR been rebased with latest in master ?

@CaerusKaru
Copy link
Member Author

@ThomasBurleson it’s all synced up

@ThomasBurleson
Copy link
Contributor

@CaerusKaru - Ok, I will review. Thank you for this PR.

- Remove default value of fxLayout to avoid confusion in Universal
- Removed browser checks from style-utils since getDOM will always
  return an implementation with the methods implemented
- Added note about getComputedStyle PR pending
- Updated RxJS to 5.5.0 and added necessary fixes
- Added better null and type checking throughout
- Updated LICENSE
@ThomasBurleson
Copy link
Contributor

@CaerusKaru - why did you use a replaySubject(1) instead of a behaviorSubject?

@CaerusKaru
Copy link
Member Author

@ThomasBurleson BehaviorSubject gives an incorrect assumption that all fxLayouts start with a row value. While it's true that fxLayout defaults to row (and still does), anything that subscribes to this value later down the chain is given a false impression.

Enter ReplaySubject(1), which is exactly like the BehaviorSubject, except without the erroneous "default" value. Now if something subscribes, it will wait for the first real value, and anything that subscribes later will still get the current value.

@CaerusKaru
Copy link
Member Author

This is not a dealbreaker for me, and I'd be happy to change it back since it doesn't appear to affect functionality. I just thought it was an improved alternative.

@CaerusKaru CaerusKaru deleted the universal branch October 23, 2017 02:54
@ThomasBurleson
Copy link
Contributor

@CaerusKaru - Thank you for this PR. Merged in SHA 8dcae02

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug(fxLayout, universal): layout direction is wrong
3 participants