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

Always render DOM for hidden wizard steps #15467

Open
wants to merge 2 commits into
base: 3.x
Choose a base branch
from

Conversation

freshleafmedia
Copy link

Description

When applying conditional hidden() reactivity to wizard steps you get weird rendering of subsequent steps on previous ones.

There is a repro repo here: https://github.com/freshleafmedia/filament-wizard-step-repro

Visual changes

Before

wizard-before-2025-01-29_12.31.31.mp4

After

wizard-after-2025-01-29_12.32.54.mp4

Functional changes

  • Code style has been fixed by running the composer cs command.
  • Changes have been tested to not break existing functionality.
  • Documentation is up-to-date.

@freshleafmedia freshleafmedia changed the title Always render hidden wizard steps Always render DOM for hidden wizard steps Jan 29, 2025
@freshleafmedia
Copy link
Author

I also noted that the hidden stepsData input filters out hidden steps but the getComponents() call doesn't return them by default.

See

collect($getChildComponentContainer()->getComponents())
->filter(static fn (\Filament\Forms\Components\Wizard\Step $step): bool => $step->isVisible())
->map(static fn (\Filament\Forms\Components\Wizard\Step $step) => $step->getId())
->values()
->toJson()

@danharrin
Copy link
Member

@freshleafmedia regarding your latest comment does something need to be changed, I don't follow what is wrong.

@danharrin danharrin added the bug Something isn't working label Jan 31, 2025
@danharrin danharrin added this to the v3 milestone Jan 31, 2025
@freshleafmedia
Copy link
Author

freshleafmedia commented Jan 31, 2025

@danharrin It's not causing a problem and doesn't need changing.

To explain: getComponents() only returns visible components, which makes the filter() call redundant.

I only mention it because it suggests the wizard was built on the assumption that getComponents() returns hidden components, which AFAIK it doesn't.

Sorry for the confusion.

@danharrin
Copy link
Member

Ah okay, can you remove the redundant filter then please? And are you sure we don't now need to include step data for hidden steps after this change?

Also, how does this problem affect Tabs which operates on a similar principle, should hidden tab content be rendered too?

@sprtk-ches
Copy link
Contributor

sprtk-ches commented Jan 31, 2025

Yeap tried it.

Closes #11084

That being said, it does render the body of hidden steps, so I'm not sure if it's something to consider. For example, if you have something that you do not want to show, it will be visible if you inspect the DOM

Here is an example
image

As you can see, the solutions step is not visible, but you can see the body of that tab in the DOM

@danharrin
Copy link
Member

danharrin commented Jan 31, 2025

Yeah I think that is an necessary evil

@archilex
Copy link
Contributor

archilex commented Jan 31, 2025

@freshleafmedia I ran into the same issue and solved it by adding wire:key="{{ $this->getId() }}" to the wrapping div of the wizard step blade file. Not sure if that's a better solution / or even fully fixes the problem for everyone, but thought I'd mention it.

Edit: I just pulled down your repo to test and indeed wire:key="{{ $this->getId() }}" fixes it in yours as well with the added benefit that we're not rendering the hidden steps.

Haven't tested if it closes #11084

@sprtk-ches
Copy link
Contributor

Ok I tried @archilex's solution and it does appear to work (also solves #11084 )

Not sure if you want to create another PR with your solution?

Anyway, up to you @danharrin but I disagree that it's a necessary evil. You could have a hidden step with sensitive information visible to certain users only that would still be rendered.

@danharrin
Copy link
Member

@sprtk-ches sure, if there is an alternative that is preferable.

@archilex
Copy link
Contributor

archilex commented Feb 2, 2025

@freshleafmedia feel free to update your PR with the alternate fix if you like. Or I can open a separate PR if you prefer.

@freshleafmedia
Copy link
Author

@archilex Adding the key seems to work but for the wrong reason, adding wire:key="{{ $this->getId() }}" gives the following:

20250204-1014

The keys are all the same, which is not what is intended. Adding wire:key="{{ $id }}" gives unique keys but then the original problem returns.

I believe the root of the problem is a conflict of control between Livewire and AlpineJS. By getting Livewire to render all steps up-front AlpineJS is solely responsible for controlling the step visibility.

@freshleafmedia
Copy link
Author

@danharrin Are you happy to merge this PR as-is?

@danharrin
Copy link
Member

How about adding the step ID into the wire:key somewhere? No, judging by the responses in the thread it doesn't sound like it's safe to render hidden steps anyway

@freshleafmedia
Copy link
Author

Setting the key doesn't resolve the problem.

I have tried wire:key="{{ $id }}", $id being the step id, but the problem persists.

The only comment which alluded to safety you dismissed as necessary evil, has that changed?

@archilex
Copy link
Contributor

archilex commented Feb 4, 2025

@freshleafmedia Dan means to append the step key to the component key: `wire:key="{{ $this->getId() }}.{{ $id }}”. That way livewire is forced to rerender the components but each key is unique.

Might want to add “step” in there. You can search the code base and see how this strategy is implemented in other places.

@freshleafmedia
Copy link
Author

@archilex Just tried wire:key="{{ $this->getId() }}.step.{{ $id }}", this makes the keys globally unique but the problem persists.

20250204-1405

@archilex
Copy link
Contributor

archilex commented Feb 4, 2025

@freshleafmedia You're right. Looks like the component key isn't changing between requests. I assumed it was since just adding {{ $this->getId() }} was working. Looks like that leaves the brute force way of either appending a random key or just using a random key wire:key="{{ \Illuminate\Support\Str::random() }}" which does appear to fix the issue.

@danharrin
Copy link
Member

I'm not going to merge in a solution with a random key, it will cause all sorts of problems on re-render

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

4 participants