Skip to content

[FIXES] Fix deprecated sass, Polish translation, types, build command #1929

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

Closed
wants to merge 33 commits into from

Conversation

Bartek20
Copy link
Contributor

@Bartek20 Bartek20 commented Jan 11, 2025

  • Fix part of vulnerabilities
  • Fix deprecated sass global functions
  • Moved from grunt-sass to grunt-contrib-sass
  • Added handling special cases in translation
  • Added injecting iti instance to input html element
  • Fix error during npm run build types generation
  • Fix generated types bug (types export utils as utils-compiled not as utils)

@Bartek20 Bartek20 changed the title Fix some problems with sass Fix deprecated sass, fix Polish translation Jan 13, 2025
@Bartek20 Bartek20 changed the title Fix deprecated sass, fix Polish translation [FIXES] Fix deprecated sass, Polish translation, types, build command Jan 16, 2025
@Bartek20
Copy link
Contributor Author

Bartek20 commented Feb 4, 2025

Hi @jackocnr,
Sorry for pinging you. When can we expect to have it merged?

I see that you released new version since this request was created but you haven't included it in release.
Can I ask in which version will it be included.

@jackocnr
Copy link
Owner

Hi there, thanks for putting this together. Unfortunately, I have very limited time to commit to open-source over the next month or so, so all I can do is merge very simple straightforward PRs e.g. basically just simple translations and LPN updates. I see you're including a translation change here, but the logic looks complicated so I'm afraid I won't be able to review this for a while.

One thing I can say is that you have included a lot of different changes in this PR, and when I have time to review it, probably in late March to be honest, I will need more information on why each change needs to happen, and why your approach fixes the issue e.g. for the Polish language change, can you describe what the current problem is, with an example, and provide a source to justify your approach here. Thanks. And apologies for the delay, but life gets in the way of open-source sometimes!

@Bartek20
Copy link
Contributor Author

Bartek20 commented Feb 18, 2025

@jackocnr np. I just wanted to know when merge can be expected.

Also here's why this changes need to happen:

  1. Fix vulneratilities - obvious
  2. Deprecated sass - deprecated function may soon be removed - @import, map (global module)
  3. Move grunt-contrib-sass - Please ignore already reverted back as fix was released on grunt-sass repo.
  4. Some languages have different form for same words in different amount of something and it can't be handled using current zero/one/more approach in counting search results
    eg. Polish: 2 wyniki but 12 wyników and 32 wyniki
    I solved it using new Function() constructor which may be considered unsafe, but input of this contructor comes directly from translations and is checked between releases so I assumed it's input is controled so should be safe.
    Alternative approach would be to add new key to translation file (something like searchResultFunction) and here implement function instead of using new Function() constructor.
  5. Inject Iti instance - Adds ability to access instance directly from DOM element (eg. document.getElementById('phoneInput').iti)
  6. Build error - Fixed error described in here and mentioned here feat: add angular component #1941.
  7. Utilities - Rn utilities are exported as intl-tel-input/utils but types file exports them as intl-tel-input/utils-compiled as it takes file name as export. I renamed file and fixed this issue (reported in TypeScript module typing for intl-tel-input/utils missing #1940)

@nicksleap
Copy link

Any updates?

@simply-alliv
Copy link

I'm also curious on an ETA about this

@simply-alliv
Copy link

Screenshot 2025-05-12 at 09 05 48

As you can see in the screenshot above, the simple but temporary fix is to disable the TypeScript error for this specific line by using a TypeScript directive comment.

@jackocnr
Copy link
Owner

jackocnr commented Aug 11, 2025

Hi @Bartek20, I can only apologise again for the delay in getting to this. I have had a lot come up in my personal life. But I'm here now.

Thanks again for putting all of this together. There are loads of great changes in here that I'm excited about merging! e.g. modernising the sass to get rid of the deprecation warnings, and the great catch on the utils-compiled filename issue.

Some feedback:

  • Re: Polish plurals handling - thanks for bringing this issue to my attention! After looking over the code, I prefer your second idea of just putting the function straight in the translation file. This way, the pluralising logic will be more readable (especially in languages like Arabic where apparently there are 6 different kinds of plurals!), and we won't need the new Function() code. Are you up for making that change?
  • Re: the build error fix in grunt/shell.js, I've done some research, and it seems that sed behaves differently on different platforms, e.g. on master, there's currently no error on MacOS, but if you make this change, then on MacOS it starts generating backup files that we don't need. I have found a different fix for this and pushed it to master, so please could you drop this change from this PR.
  • Finally, if possible, could you rebase and drop the merge commits. Currently, when I look at the changes in this PR, it includes changes to the flag sprites, which shouldn't be there - I guess they're somehow bundled into the merge commits. Pesky merge commits! 🙃

I'm aware it has been several months since you wrote this, so if you don't have the time/inclination to make these changes, then just let me know and I can try to do it myself. Cheers.

@Bartek20
Copy link
Contributor Author

Hi @jackocnr,
I modified translation to work from new parameter searchResultsText, and managed to update rest of commits to latest from your repo but it seems like I made some mistakes on the way so I had to update libphonenumber to latest in last commit and not in history (only utils were updated version stayed old). I usually only use vscode commit/push button so I don't know how to change that to be correct. I also don't know how to drop merge commits from previous updates (I only managed to merge latest changes as linear update).

@jackocnr
Copy link
Owner

@Bartek20 thanks for this. The commit history on this branch is now very messy - somehow there are now 33 commits in here, including lots of old ones authored by me, and several of your own commits seem to have been duplicated as well. For this reason, I have manually squashed all of your changes into a single commit in master which you can now see here. Thanks again for your great work on this!

@jackocnr jackocnr closed this Aug 20, 2025
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.

5 participants