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

Carousel updates #437

Merged
merged 4 commits into from
Mar 10, 2021
Merged

Carousel updates #437

merged 4 commits into from
Mar 10, 2021

Conversation

gomjabar6
Copy link
Contributor

@gomjabar6 gomjabar6 commented Mar 10, 2021

Hello!

Lots of updates for the carousel component here. This was all spawned by an exception caused when the parent of a carousel component re-rendered while the carousel was in the middle of an animation. I found this by subscribing to the ActiveIndexChanged event on the BSCarouselControl. You can replicate this on any of your sample carousels by subscribing as so:

<BSCarouselControl CarouselDirection="CarouselDirection.Previous" ActiveIndexChanged="@indexChanged" />

This leads to errors in both server and client side cases. Here is a shot of the server side error:
image

Several things happened in this case:

  1. Clicking on the BSCarouselControl element fires the slide transition code and then immediately fires the ActiveIndexChanged event.
  2. The parent, seeing that an event has happened, calls SateHasChagned() and the BSCarousel componet re-renderes.
  3. The BSCarousel component would automatically flush the list of Items due to the NumberOfItems parameter being re-set from the parent:
 [Parameter]
  public int NumberOfItems
  {
      get => _numberOfItems;
      set
      {
          if (_lastNumberItems != _numberOfItems)
          {
              CarouselItems.Clear();
          }
          _numberOfItems = value;
      }
  }
  1. The animation would continue it's task and end up hitting one of the lines that assumed there were still Carousel items
new Task(async () =>
  {
      AnimationRunning = true;
      await CarouselItems[ActiveIndex].Clean().ConfigureAwait(false);
      CarouselItems[ActiveIndex].Next = true;
      await CarouselItems[ActiveIndex].StateChanged().ConfigureAwait(false);
      await Task.Delay(300).ConfigureAwait(false);  // Gives animation time to shift and be ready to slide.
      CarouselItems[ActiveIndex].Left = true;
      CarouselItems[oldindex].Left = true;
      await CarouselItems[ActiveIndex].StateChanged().ConfigureAwait(false); // makes sure there is no gap
      await InvokeAsync(StateHasChanged).ConfigureAwait(false);
  }).Start();
  1. Since the CarouselItems list has been cleared, the component would throw an exception.

I've re-written the BSCarousel initialization to avoid clearing the existing CarouselItems so that if something out of thread tried to access the list it would still have children to index.

[Parameter] public int NumberOfItems { get; set; }
....
protected override void OnParametersSet()
{

    if (CarouselItems == null)
    {
        CarouselItems = new List<BSCarouselItem>();
    }

    if (CarouselIndicatorItems == null)
    {
        CarouselIndicatorItems = new List<BSCarouselIndicatorItem>();
    }

......
}

While I was in there, I have also re-written the animation system to use the BSCarousel as the main "driver" of changes. Instead of the child components directly modifying the ActiveIndex, they instead call helper functions in BSCarousel.

Before:
From BSCarouselControl.Razor.CS:

protected async Task OnClick()
{
    if (Parent.AnimationRunning) return;
    if (CarouselDirection == CarouselDirection.Previous)
    {
        Parent.Direction = 1;
        Parent.ResetTimer();
        Parent.ActiveIndex = Parent.ActiveIndex == 0 ? NumberOfItems - 1 : Parent.ActiveIndex - 1;
    }
    else
    {
        Parent.Direction = 0;
        Parent.ResetTimer();
        Parent.ActiveIndex = Parent.ActiveIndex == NumberOfItems - 1 ? 0 : Parent.ActiveIndex + 1;
    }
    await ActiveIndexChanged.InvokeAsync(Parent.ActiveIndex).ConfigureAwait(false);
}

After:

From BSCarouseLControl.Razor.CS:
 protected async Task OnClick()
  {
      if (Parent.AnimationRunning) return;
      if (CarouselDirection == CarouselDirection.Previous)
      {
          await Parent.GoToPrevItem().ConfigureAwait(true);
      }
      else
      {
          await Parent.GoToNextItem().ConfigureAwait(true);
      }
      await ActiveIndexChanged.InvokeAsync(Parent.ActiveIndex).ConfigureAwait(true);
  }

From BSCarouselControl
public async Task GoToNextItem()
{
    if (AnimationRunning) return;
    ResetTimer();
    ActiveIndex = ActiveIndex == NumberOfItems - 1 ? 0 : ActiveIndex + 1;
    await DoAnimations().ConfigureAwait(true);
}

public async Task GoToPrevItem()
{
    if (AnimationRunning) return;
    ResetTimer();
    ActiveIndex = ActiveIndex == 0 ? NumberOfItems - 1 : ActiveIndex - 1;
    await DoAnimations().ConfigureAwait(true);
}

Making BSCarousel the main driver means we can have everything happening in the same thread (except the Timer, no luck there). This means DoAnimations does not need to span the complex tasks as it was before. I don't necessarily believe this NEEDS to happen. The previous change means you won't have the out-of-thread issues but personally I think it is way cleaner and easier to debug. Happy to take this part out if you don't like it. New DoAnimations:

private async Task DoAnimations()
{
    if (CarouselItems.Count == 0) return;

    if (_timerEnabled)
    {
        Timer.Stop();
        Timer.Interval = CarouselItems[ActiveIndex].Interval;
    }

    AnimationRunning = true;
    CarouselItems[ActiveIndex].Clean();

    Direction = GetDirection();

    //Add new item to DOM on appropriate side
    CarouselItems[ActiveIndex].Next = Direction == 0;
    CarouselItems[ActiveIndex].Prev = Direction == 1;
    this.StateHasChanged();
    await Task.Delay(300).ConfigureAwait(true); //Ensure new item is rendered on DOM before continuing

    //Trigger Animation
    CarouselItems[ActiveIndex].Left = Direction == 0;
    CarouselItems[_prevIndex].Left = Direction == 0;

    CarouselItems[ActiveIndex].Right = Direction == 1;
    CarouselItems[_prevIndex].Right = Direction == 1;
    this.StateHasChanged();
        
}

If you notice, I no longer need all the various components to specify the direction, we can determine the direction by comparing the new index against the old index:

private int _activeIndex { get; set; }
private int _prevIndex { get; set; }
public int ActiveIndex
{
    get => _activeIndex;
    set {
        _prevIndex = _activeIndex;
        _activeIndex = value;
    }
}
...
private int GetDirection()
{
  if (_prevIndex == 0)
  {
      if (ActiveIndex == NumberOfItems - 1)
      {
          return 1;
      } else
      {
          return 0;
      }
  }

  if (_prevIndex == NumberOfItems - 1)
  {
      if (ActiveIndex == 0)
      {
          return 0;
      } else
      {
          return 1;
      }
  }

  if (ActiveIndex > _prevIndex)
  {
      return 0;
  } else
  {
      return 1;
  }
}

Centralizing all the code means we can do some cool stuff where clicking on the BSCarouselIndicatorItem can actually transition directly to the right slide (just like in bootstrap).

From BSCarouselIndicatorItem.Razor.CS:
protected async Task OnClick()
{
    await Parent.GoToSpecificItem(Index).ConfigureAwait(true);
    await ActiveIndexChangedEvent.InvokeAsync(Index).ConfigureAwait(true);
}

From BSCarousel.Razor.CS:
public async Task GoToSpecificItem(int index)
{
    if (AnimationRunning) return;
    ResetTimer();
    ActiveIndex = index;
    await DoAnimations().ConfigureAwait(true);
}

Also, having all the logic in BSCarousel means we can subscribe to index changes directly on BSCarousel instead of having to do it on the Indicator/Controls in the first place:

From BSCarousel.Razor.CS:
[Parameter] public EventCallback<int> ActiveIndexChangedEvent { get; set; }
...
public async Task AnimationEnd(BSCarouselItem sender)
{
    if (sender == CarouselItems[ActiveIndex])
    {
        AnimationRunning = false;

        CarouselItems[_prevIndex].Clean();
        CarouselItems[ActiveIndex].Clean();
        CarouselItems[ActiveIndex].Active = true;
        this.StateHasChanged();

        if (_timerEnabled)
            Timer.Start();

        await ActiveIndexChangedEvent.InvokeAsync(ActiveIndex).ConfigureAwait(false);
    }
}

Updated carousels1 example:
<BSCarousel NumberOfItems="@items.Count" ActiveIndexChangedEvent="@indexChanged">
    ...
</BSCarousel>

Also while in there, I have removed some methods that were not referenced and consolidated the NumberOfItems from issue #379. Carousel examples are updated to match the changes.

Overall this should be a seamless upgrade for carousels. The one gotcha will be the NumberOfItems change from issue #379 since, after updating, the lower level components will not recognize the NumberOfItems parameter.

Lots of changes here, I'm sorry this isn't smaller PRs. Happy to refactor individual features if you need them.

@jbomhold3
Copy link
Collaborator

Testing changes and will check in and put out a preview build if all gos well.

@jbomhold3 jbomhold3 added Breaking Change Requires code changes Important Everyone should read labels Mar 10, 2021
@jbomhold3 jbomhold3 merged commit 5591e92 into chanan:master Mar 10, 2021
@jbomhold3
Copy link
Collaborator

This also fixes #380
Moves ActiveIndexChangedEvent="@indexChanged" to BSCarousel.
Removes NumberOfItems="@items.Count" from BSCarouselControl

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Requires code changes Important Everyone should read
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants