bestguy / sveltestrap

Bootstrap 4 & 5 components for Svelte
https://sveltestrap.js.org
MIT License
1.31k stars 183 forks source link

test: improve alert #375

Closed DaniAcu closed 3 years ago

DaniAcu commented 3 years ago

Changelog

DaniAcu commented 3 years ago

@bestguy I noticed that we are using a prop to the toggle event which could be onClose. However I wondering why to use a dispatch as svelte recommend instead to pass a function as parameters 🤔.

I'm a React fan so I like how this looks like but I'm thinking if we could do this more in a svelte way.

bestguy commented 3 years ago

Great, thank you @DaniAcu ! I'll take a look, that makes sense

DaniAcu commented 3 years ago

Let me know if the tests looks good or I need to change something. I could continue contributing with this kind of things 😄

bestguy commented 3 years ago

Thanks, it looks good so far, but I want to be cautious on adding babel changes in case it impacts the published library. It should be fine, but I'll confirm over the weekend. Thank you for the help 👍

DaniAcu commented 3 years ago

@bestguy I noticed that the bundle increase 10KB with the babel plugin. I could configure a specific babel config for testing if that work for you 😄

bestguy commented 3 years ago

Yes that sounds great. Are you sure async was not supported already in tests?

DaniAcu commented 3 years ago

Mmm pretty sure, it was failing because the regeneratorRuntime was not defined which means that the babel pollyfill was not included. So I just add the regenerator plugin that we need to prevent to add all the pollyfills

bestguy commented 3 years ago

Hi @DaniAcu , I think this change to babel.config.js might work without additional plugins:

module.exports = {
  presets: [[
    '@babel/preset-env',
    {
      "targets": {
        "node": "current"
      }
    }
  ], '@babel/preset-typescript']
};
DaniAcu commented 3 years ago

Nice, I will continue adding other config or maybe use NODE_ENV, because we don't want to use node as target for a web component library, it should continue using web as target. Thanks for the suggestion 😊

bestguy commented 3 years ago

Oh yes true, in that case I think can use the babel env test blocks for this, instead of referencing NODE_ENV in code.

DaniAcu commented 3 years ago

@bestguy haha yeah! I pushed that at the same time that you mention haha. I have a problem with package-lock.json. Let me take a look

DaniAcu commented 3 years ago

Now it should be fine 😄

DaniAcu commented 3 years ago

If this is fine, I'll continue cleaning other tests and trying to add new ones to have a better quality in our code.

bestguy commented 3 years ago

Thanks @DaniAcu, I'll sync and try this evening. I might have a suggestion on the babel config but let me confirm first 👍

DaniAcu commented 3 years ago

Any update? 🤔