morajabi / styled-media-query

💅💍 Better media queries for styled-component
MIT License
1.32k stars 53 forks source link

[TypeScript] Wrong Type Definition #23

Closed raulfdm closed 4 years ago

raulfdm commented 4 years ago

Hello everyone 👋

Expected Behavior

Be able to pass size (e.g. 1024px) to all media query generators without any TypeScript complaint.

Actual Behavior

The current type definition only allow us to pass the breakpoint keys (image below). If you pass a size, it'll work fine but the typescript compiler will complain about this unexpected value.

Screenshot 2020-01-24 at 11 57 34

Steps to Reproduce the Problem

  1. Access this sandbox https://codesandbox.io/s/silent-forest-rgzhj
  2. Wait until loads completely
  3. Check Box component media query warning

Specifications

raulfdm commented 4 years ago

Can we merge and release? =D https://github.com/morajabi/styled-media-query/pull/21/files

dawidjaniga commented 4 years ago

Hi @morajabi Can you suggest ETA of the above merge?

raulfdm commented 4 years ago

I think I have a better solution to that. When I opened this issue I didn't know about function overloading but I think we could fix that doing some changes like this:

Screenshot 2020-06-16 at 13 18 48 Screenshot 2020-06-16 at 13 18 59

In that way, we'll be able to get intellisense and also pass a string because there are 2 possible versions for each function.

TypeScript playground: https://www.typescriptlang.org/play/index.html#code/FAimDsFcFsAIBFQDMCGkA2AXAQgJ1CgNYAOA9gJbiYDOsA3sLLABaQDmosAvLAOSsdeAGkax0KXB259xk0MNFNooACbkYPXsrUwFTJtWgp06TYePoFAXxCVMoXKgDGnALKryKAOIQHKTKS4ADx4BCQUVNRCsAAqzKDKAHz0iuig1NRxKOBBAAq4pMTUiQAUivoA9ABUsAByAPKwAKq1AJL1tbAA6q0xABKwAMoxAEqttV6wVRXlTABG+ERkdgBcsNSYuJRsIvqwAJRr2QCeANyiaRlZOfmFxWV7TNV1jS3tnT39Q6Pjk9OzsAWYWWVDWoSWERou30hxO5yYbEW9lw1zWeQKRVKAOebw63V6A2GYwmUxmj0Bi3Cq1g4KpkVgAB91ptttCmPtuMk4eU5qBMAB3UAQNG3TEPR5IcgbMGUkFQgHUUBOUjgFQy4GQqLlDlcLngM7AGzAFRK2ScZXgDawbSeNbuNTeXy4fyBIKEUDHUhIWCYY7EUBehDINBYWlyqKwE6Jc7AX3+yNzJzSd2e71xgPexCoDA4WWakA2lAAOkumWY2RK-HY8n2wELJfSZYrvAAjAAGNvEAAevFr9cRBGR1xK+yAA

dawidjaniga commented 4 years ago

Got you @raulfdm Sounds like a good idea 👍 My only concern is you've created PR almost half a year ago, but no answer was given from the time. cc: @morajabi

raulfdm commented 4 years ago

indeed... I had to fix this locally. It would be so nice having this by default.

@morajabi maybe you're busy with other things and that's totally fine but maybe you can add other person who wants to contribute to this project as collaborator?

morajabi commented 4 years ago

Hey @raulfdm, I invited you to the project as a collaborator. I'd love it if you could try the PR and see if it fixes the typescript issue, let us know? In that case, let me know and you can merge it then I'll publish to npm.

raulfdm commented 4 years ago

@morajabi I've made the proposal.

What do you think about automating the deployment publish? I'll open an issue to create this proposal but in general, my idea is:

  1. Setup Github Actions to do some validations like lint, tests, prettier check, etc;
  2. Automatically publish when gets merged to master using semantic release

I have a few libraries automated within this process and that's nice because it's only about merging stuffs and everything will be automatically done for us.

KesleyDavid commented 4 years ago

Good morning folks, any release forecast? I'm in the middle of a project, when I started coding the responsive I discovered this detail

KesleyDavid commented 4 years ago

@raulfdm Good morning friend, any news about the update? i need a lot for a project

raulfdm commented 4 years ago

hey @KesleyDavid .... well, the PR was merged today and needs to be released.

Since this project does not have a CI/CD with auto release I think @morajabi needs to do that.

ogwtkhr commented 3 years ago

@morajabi Hello. I am looking forward to the release of this fix. So, Will this fix be published to npm?