-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Refactor webL10n.js for Improved Readability and Maintainability #4445
base: master
Are you sure you want to change the base?
Conversation
❌ Some Jest tests failed. Please check the logs and fix the issues before merging. Failed Tests:
|
We need to think about whether or not we want to maintain these changes locally. But it does make the code easier to read. |
I understand that the i10n library might change in the near future. However, since I’m very interested in this idea, I believe these refactoring changes will make it easier for anyone working on this part of the project to understand the existing implementation. Also, I noticed that webL10n.sugarizer.js could benefit from similar refactoring, so I’ve also started working on improving it. Let me know if you have any thoughts on this! |
OK. I'll test these changes. |
just following up, any progress on the review? |
❌ Some Jest tests failed. Please check the logs and fix the issues before merging. Failed Tests:
|
Sorry about that, this commit should be in another branch under working. |
✅ All Jest tests passed! This PR is ready to merge. |
…ackend from package.json and package-lock.json
❌ Some Jest tests failed. Please check the logs and fix the issues before merging. Failed Tests:
|
@Ashrafmuhmed perhaps you can look into why the "SaveInterface.test.js" test is failing? |
I suspect a rebase would fix it. |
But I still have my doubts about making these changes. We should be using the min version of the code here and let the upstream un-minimized version be where users go to explore how it works. |
Understood. If we use the minified version here, where should we place the unminimized version for users to explore? Any suggestions? |
Maybe we should add a README.md file to the lib directory that explains what min files are and how to find the upstream versions? But no need to keep the expanded versions here. |
Agreed. Let's add the README.md and clarify everything there. Should we also create a folder named upstream-version to include the unminified code for reference? We can start making these changes right away. |
✅ All Jest tests passed! This PR is ready to merge. |
Description
This pull request refactors the webL10n.js file to improve readability and maintainability. The following changes have been made: