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: Server-Side pre-rendering issue with Bar calling JSInterop #1027

Merged
merged 3 commits into from Jun 26, 2020

Conversation

MitchellNZ
Copy link
Contributor

@stsrki opening PR as a patch for dev091.
I can change the target branch if thats not what you want.

Fixes #1026

@stsrki stsrki changed the base branch from dev091 to sup091 June 26, 2020 08:22
@stsrki
Copy link
Collaborator

stsrki commented Jun 26, 2020

Looks good. The only issue I have is that by the time OnFirstAfterRenderAsync is executed and you change Visible state, possibly it would need to redraw by calling StateHasChanged.

@stsrki
Copy link
Collaborator

stsrki commented Jun 26, 2020

Oh yeah, I switched to new branch. From now dev branches are for new stuff, while sup will be used for support of published version(s).

@stsrki stsrki added this to the 0.9.1 milestone Jun 26, 2020
@stsrki stsrki linked an issue Jun 26, 2020 that may be closed by this pull request
@MitchellNZ
Copy link
Contributor Author

Looks good. The only issue I have is that by the time OnFirstAfterRenderAsync is executed and you change Visible state, possibly it would need to redraw by calling StateHasChanged.

I thought this too, but it seemed work fine when I tested it (possibly due to the VisibleChanged being bound)?
Maybe you could please double check this before merging!

@stsrki
Copy link
Collaborator

stsrki commented Jun 26, 2020

I would if I would know what to test 😅. This is your area.

For example what is the usage of lastBrokenState?

One optimization I think will be good is to have if the Visible need to be changed and then refresh

if (  Visible != something )
{
    Visible = something;
    StateHasChanged();
}

@MitchellNZ
Copy link
Contributor Author

😅
The purpose of this code is control the initial state (collapsed or not) depending on the breakpoint that has been defined.
It should only run on the initial render, and RegisterBreakpointComponent will take care of the breakpoint state from there.

I think I will rename lastBrokenState to isBroken..

It has two simple rules:

  • If the Bar "is broken" (isBroken= true) then we want it to collapse (Visible = false).
  • If the Bar "is not broken" (isBroken= false) then we want it to be shown (Visible = true).

So I think the following code will fit with your suggestion of adding StateHasChanged():

isBroken = BreakpointActivatorAdapter.IsBroken( Breakpoint, await JSRunner.GetBreakpoint() );
if ( Visible == isBroken )
{
    Visible = !isBroken;
    StateHasChanged();
}

I will update it!

@stsrki
Copy link
Collaborator

stsrki commented Jun 26, 2020

Maybe the naming should be collapsed or something in a way?

Also it could be good to write a comment to explain what it do and why it's used for...

@MitchellNZ
Copy link
Contributor Author

To me, isBroken is a little bit more clear, since it is tracking the current breakpoint state.
But I am happy to change it further if you think otherwise :)

I have updated with some comments too.. its a difficult thing to explain, so I hope it makes sense 😅

@stsrki
Copy link
Collaborator

stsrki commented Jun 26, 2020

I think it's OK now. The comments are for us in the future so we know what's going on :)

Good work as always, Thank you!

@stsrki stsrki merged commit 5c2e002 into Megabit:sup091 Jun 26, 2020
@stsrki stsrki mentioned this pull request Jun 29, 2020
7 tasks
stsrki pushed a commit that referenced this pull request Jun 30, 2020
* fix: Server-Side pre-rendering issue with Bar calling JSInterop

* fix: Add StateHasChanged() and rename lastBrokenState for Vertical bar breakpoint management

* fix formating

Co-authored-by: Mladen Macanovic <mladen.macanovic@pebble.tv>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Support
  
Awaiting triage
Development

Successfully merging this pull request may close these issues.

Bar issue with Server-Side pre-rendering
2 participants