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

Version 2 #152

Merged
merged 66 commits into from
Apr 28, 2022
Merged

Version 2 #152

merged 66 commits into from
Apr 28, 2022

Conversation

piotrpospiech
Copy link
Contributor

@piotrpospiech piotrpospiech commented Feb 3, 2022

Info: Marked as Draft to prevent from merging

This PR is focused on the first three parts of the issue #144 . Documentation is still in work in progress. It will be completed after all changes from v2 were implemented.

Main changes:

  • Removed locales, currency data, and all functions that used this data. Locale won't be checked with available locales anymore. It's checked by regex. Available formats: xx and xx-XX, where x is lowercase letter and X is uppercase letter.
  • Removed React integration. Documentation still needs more detailed instructions.
  • Removed createTranslator, createReactiveTranslator, purify and parseNumber functions.
  • Removed tests that used deleted code.
  • Removed unused format function.
  • Black-boxed parts of logic from getTranslation function.

@piotrpospiech piotrpospiech marked this pull request as draft February 3, 2022 14:39
Copy link
Contributor Author

@piotrpospiech piotrpospiech left a comment

Choose a reason for hiding this comment

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

@zendranm , some suggestions from me below

source/common.ts Outdated Show resolved Hide resolved
source/common.ts Outdated Show resolved Hide resolved
source/common.ts Outdated Show resolved Hide resolved
source/common.ts Outdated Show resolved Hide resolved
source/common.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@radekmie radekmie left a comment

Choose a reason for hiding this comment

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

Good job! Let's schedule a meeting after you'll address all of the comments and TODOs, as I have a couple of ideas when it comes to the documentation.

Also, could you already look into CI?

source/common.ts Outdated Show resolved Hide resolved
source/common.ts Outdated Show resolved Hide resolved
source/server.ts Outdated Show resolved Hide resolved
source/common.ts Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@piotrpospiech
Copy link
Contributor Author

@radekmie

It'd be better to add puppeteer to standard dependencies and install them at once.

Puppeteer has to be installed as Meteor test-app dependency, so it cannot be added to standard dependencies.

source/modules.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@zendranm zendranm changed the title [WIP] Version 2 Version 2 Apr 11, 2022
@zendranm zendranm marked this pull request as ready for review April 11, 2022 06:42
package.json Outdated Show resolved Hide resolved
source/compiler.ts Outdated Show resolved Hide resolved
tests/tests.bash Outdated Show resolved Hide resolved
Comment on lines +26 to +29
- name: Install Meteor
env:
METEOR_VERSION: ${{ matrix.meteor-version }}
run: curl https://install.meteor.com/?release=$METEOR_VERSION | /bin/sh

Choose a reason for hiding this comment

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

Would probably be worth caching both meteor and npm install dependencies - doesn't have to be within this PR though.

source/common.ts Outdated Show resolved Hide resolved
source/server.ts Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved

```bash
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have tests for all of the examples here? We should.

README.md Show resolved Hide resolved
@radekmie
Copy link
Contributor

As discussed today: let's focus on checking the normalization (#152 (comment)) and we could release an alpha. The rest (caching on CI, more tests) can be done later.

@zendranm
Copy link
Member

As discussed today: let's focus on checking the normalization (#152 (comment)) and we could release an alpha. The rest (caching on CI, more tests) can be done later.

@radekmie The normalization was tested and it works. For now only one param form the queryParams is being used in the loadLocale function - file type json. Looks like all the other params specified in queryParams are not currently handled. It can be taken care of later on.

@zendranm zendranm requested a review from radekmie April 27, 2022 12:08
@piotrpospiech piotrpospiech merged commit bae56f8 into master Apr 28, 2022
@piotrpospiech piotrpospiech deleted the version-2 branch April 28, 2022 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants