Skip to content

Conversation

@thekiba
Copy link
Contributor

@thekiba thekiba commented Sep 1, 2020

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows Conventional Commits
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Refactoring (no functional changes, no api changes)
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@thekiba thekiba requested a review from waterplea as a code owner September 1, 2020 12:29
@waterplea
Copy link
Member

I wonder if it should be named NETWORK_INFORMATION or CONNECTION. @MarsiBarsi what do you think?

@thekiba thekiba force-pushed the feat/network-information-token branch from 587394b to eed0970 Compare September 1, 2020 12:42
@thekiba
Copy link
Contributor Author

thekiba commented Sep 1, 2020

@waterplea NETWORK_INFORMATION is more informative for me instead of CONNECTION 😁

Please check the pr again, I force pushed the changes

@MarsiBarsi
Copy link
Contributor

both options are okay for me 👍

@thekiba thekiba force-pushed the feat/network-information-token branch from eed0970 to a1b8148 Compare September 1, 2020 13:25
@thekiba
Copy link
Contributor Author

thekiba commented Sep 1, 2020

@waterplea I have reverted the change of returns undefined instead of null in the token factory for the tests to pass

@waterplea
Copy link
Member

@thekiba instead you can add a second test where you would mock NAVIGATOR token with an object missing connection property. Test would be: "Injects null in unsupported browsers"

@thekiba thekiba force-pushed the feat/network-information-token branch from a1b8148 to c44899c Compare September 1, 2020 13:38
@thekiba
Copy link
Contributor Author

thekiba commented Sep 1, 2020

It's a success now 🙌

@waterplea
Copy link
Member

All looks well now. I wonder if there's a way not to force everybody to install those types if they do not use this token 🤔

@waterplea
Copy link
Member

@MarsiBarsi what if we bundle it? It's just types, shouldn't really matter. What do you think?

@MarsiBarsi
Copy link
Contributor

@waterplea I think it is a good idea. Let's make the type with "TODO to remove" after adding TS support

@waterplea
Copy link
Member

Took the liberty to do those changes, related to types bundling. Looks like it works fine now, I tried a pre-release version on my pet project. Thank you, @thekiba !

@waterplea waterplea merged commit 492018e into taiga-family:master Sep 2, 2020
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.

3 participants