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

test: improve alert #375

Merged
merged 4 commits into from
Sep 14, 2021
Merged

test: improve alert #375

merged 4 commits into from
Sep 14, 2021

Conversation

DaniAcu
Copy link
Contributor

@DaniAcu DaniAcu commented Sep 4, 2021

Changelog

  • Add @babel/plugin-transform-runtime to be able to use async/await
  • Add @testing-library/jest-dom/extend-expect to be able to use the expect extentions
  • Update alert test to cover all the cases
    • Prevent use classes to get the elements, instead I used the queryByRole.
    • Add dismissible, controllable and without fade cases

@DaniAcu
Copy link
Contributor Author

DaniAcu commented Sep 4, 2021

@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
Copy link
Owner

bestguy commented Sep 4, 2021

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

@DaniAcu
Copy link
Contributor Author

DaniAcu commented Sep 4, 2021

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

@bestguy
Copy link
Owner

bestguy commented Sep 4, 2021

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
Copy link
Contributor Author

DaniAcu commented Sep 6, 2021

@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
Copy link
Owner

bestguy commented Sep 6, 2021

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

@DaniAcu
Copy link
Contributor Author

DaniAcu commented Sep 6, 2021

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
Copy link
Owner

bestguy commented Sep 7, 2021

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
Copy link
Contributor Author

DaniAcu commented Sep 7, 2021

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
Copy link
Owner

bestguy commented Sep 7, 2021

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
Copy link
Contributor Author

DaniAcu commented Sep 7, 2021

@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
Copy link
Contributor Author

DaniAcu commented Sep 7, 2021

Now it should be fine 😄

@DaniAcu
Copy link
Contributor Author

DaniAcu commented Sep 7, 2021

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
Copy link
Owner

bestguy commented Sep 7, 2021

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

@DaniAcu
Copy link
Contributor Author

DaniAcu commented Sep 11, 2021

Any update? 🤔

Copy link
Owner

@bestguy bestguy left a comment

Choose a reason for hiding this comment

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

Thanks for your patience, one question.

babel.config.js Show resolved Hide resolved
Copy link
Owner

@bestguy bestguy left a comment

Choose a reason for hiding this comment

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

Great thank you!

@bestguy bestguy merged commit e2f4611 into bestguy:master Sep 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants