Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Implemented Step & Stepper handling without user-provided store. #304

Conversation

Sarenor
Copy link
Contributor

@Sarenor Sarenor commented Sep 28, 2022

First take on a refactored Stepper & Step component without external Store & index input.
Thanks for the Repl-Examples, I'd have missed the toggleChild usecase on my own!

Should there still be a way for the active step to be passed outside the component? If yes, how should we go about that?

Also, this probably needs a bit more tests than it currently has now, I'll implement those as soon as we're clear that this is the wanted functionality?

@endigo9740
Copy link
Contributor

Hey @Sarenor thanks for taking a stab at this one. Unfortunately I'm seeing a few issues. Some user facing, and some dev related.

Unfortunately it looks like you're running into the same issue I had, in that this works, but causes this really annoying issue on load:

stepper-load

I had it on my docket to dig a bit deeper and see if I could figure out a solution for this. I suspect it's due to the following:

  • Using data that's generated on load, rather than hard set from the start
  • The animation effects trigger when the if conditionals are met, which is triggered due to above.

Additionally, I haven't been able to reproduce it, but I seem to recall there was an issue with the calculated values if Vite triggers HMR from updating the code within the Stepper/Step. Which would related to adding/removing children if we were to support that. The indexes would just offset after a reload - so it would be like 0, 1, 2, but then after refresh would be 4, 5, 6. It was kind of mess, but might not affect end users.

Additionally it doesn't look like you're adhering to strict typing, so I'm seeing some Typescript warnings.

Screen Shot 2022-09-28 at 10 26 23 AM

Screen Shot 2022-09-28 at 10 27 48 AM

Finally, this isn't something you would be aware of, but we do have kind of a code style pattern we follow. I'm working with Rhys to better document this and will provide it soon.

For now I've gone ahead and made a few of these changes myself. See my newest commit. This focuses on adding types and adjusting the code style a bit. Rough order is:

  • Declarations towards the top
  • Functions in the middle
  • Reactive props as the end (given these are typically "fed" from above)

For now, see if you can make any headway with the loading issue. Otherwise we might have to put this one on the back burner until I have more time to help debug. It's frustrating stuff I know :/

@endigo9740 endigo9740 marked this pull request as draft September 28, 2022 15:54
@endigo9740
Copy link
Contributor

FYI I've also converted this PR to a draft, to indicate it's still a work in progress.

@Sarenor
Copy link
Contributor Author

Sarenor commented Sep 29, 2022

I am not sure if we can get that flicker to go away.
Since the register-Action will only be called after adding the Step-Element to the DOM there's always going to be a flicker, unless we find a clever way to defer the rendering until the action has been called.
I'd still consider myself a Svelte beginner, so I'm not sure if something like that exists, but I'd bet against it...

Bit of a chicken and egg problem, we need the node binding so we can't use beforeUpdate, but we don't want the node to be visible until after we've run our code so afterUpdate isn't a solution either.

@endigo9740
Copy link
Contributor

Yeah, I think there may be a way to work with it, but I'll need to be able to devote more time to it. I'd say for now we leave this be and perhaps next week I'll have a chance to dig deeper. But thanks for giving it a go either way!

export let locked: boolean = false;

// Context
export let dispatch: any = getContext('dispatch');
export let color: any = getContext('color');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be string types.

@endigo9740
Copy link
Contributor

@Sarenor See my notes here:
#222

@endigo9740 endigo9740 closed this Oct 9, 2022
@Sarenor Sarenor deleted the chore/Issue-222-Drop-Index-Props-From-Stepper branch March 28, 2023 13:22
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants