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(ivy): use renderer as a source of truth for style/class debugging #33700

Closed
wants to merge 2 commits into from

Conversation

@matsko
Copy link
Member

matsko commented Nov 8, 2019

Prior to this fix, the styling merge algorithm would intercept all
style/class writes and collect them into to map using a built-in
StyleApplyFn function. This setup does works for the styling
merge algorithm, but not for the direct write algorithm. For this
reason the renderer needs to be used instead because both algorithms
use that when applying style/class values.

@googlebot googlebot added the cla: yes label Nov 8, 2019
@matsko matsko force-pushed the matsko:debug_styling_use_renderer branch from 4717983 to 5837aee Nov 11, 2019
@ngbot ngbot bot added this to the needsTriage milestone Nov 11, 2019
@matsko matsko marked this pull request as ready for review Nov 11, 2019
@matsko matsko requested a review from angular/fw-core as a code owner Nov 11, 2019
@matsko matsko force-pushed the matsko:debug_styling_use_renderer branch from 5837aee to 402ab54 Nov 11, 2019
…gging

Prior to this fix, the styling merge algorithm would intercept all
style/class writes and collect them into to map using a built-in
`StyleApplyFn` function. This setup does works for the styling
merge algorithm, but not for the direct write algorithm. For this
reason the renderer needs to be used instead because both algorithms
use that when applying style/class values.
@matsko matsko force-pushed the matsko:debug_styling_use_renderer branch from 402ab54 to baf5255 Nov 11, 2019
listen(target: any, eventName: string, callback: (event: any) => boolean | void): () => void {
// this method is only used to trigger the check for the renderer
// as a procedural renderer to pass...
return () => {};

This comment has been minimized.

Copy link
@mhevery

mhevery Nov 11, 2019

Member

Could you add throw new Error('DebugStyle Mock does not implement') to all of the methods which are not needed?

}
}

class MockRendererForStyling {

This comment has been minimized.

Copy link
@mhevery

mhevery Nov 11, 2019

Member

As of right now you don't have type safety. I think class MocRenderForStyling implements Renderer3 and than add throw new Error('DebugStyle Mock does not implement') to all of the methods which are not needed?

*/
import {StylingMapArray} from '../interfaces/styling';

const enum Char {

This comment has been minimized.

Copy link
@mhevery

mhevery Nov 11, 2019

Member

I think this file needs some unit tests (in additional to the integration tests which you have)

@matsko matsko closed this Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.