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

Fix unicode fraction conversion #2498

Open
christianlupus opened this issue Sep 12, 2024 · 6 comments
Open

Fix unicode fraction conversion #2498

christianlupus opened this issue Sep 12, 2024 · 6 comments
Labels
bug Something isn't working Frontend Issue or PR related to the frontend code good first issue Good for newcomers

Comments

@christianlupus
Copy link
Collaborator

Description
When using unicode fractions, the web UI reports the ingredient to not be parsable but in fact it is parsable and correctly calculated.

Reproduction
Steps to reproduce the behavior:

  1. Create or edit a recipe
  2. Add an ingredient with a unicode fraction, e.g. ¾
  3. Save and view the recipe
  4. Change the recipe yield to trigger recalculation

Expected behavior
No warning is shown to the user

Actual behavior
A warning sign is shown to the user indicating a problem with the calculation (see screenshots).

Screenshots
grafik

After changing the yield amount:
grafik

Browser
Firefox

Versions
Nextcloud server version: 30
Cookbook version: 0.11.1
Database system: MySQL


Maybe @Weissnix4711 or @j0hannesr0th, can you have a look at this? Ideally, I would say that the same Regex is used to check and to do the actual calculation to avoid such conflicts. That would mean some restructuring but should be managable.

@christianlupus christianlupus added bug Something isn't working good first issue Good for newcomers Frontend Issue or PR related to the frontend code labels Sep 12, 2024
@j0hannesr0th
Copy link
Contributor

Hi @christianlupus I'll have a look at this one.

Do you want to support the common Unicode fractions or custom Unicode fractions like ⁷⁷⁄₇₈ too?

@Weissnix4711
Copy link
Contributor

Uhh, maybe I fucked the regex

@christianlupus
Copy link
Collaborator Author

In the PR there was a discussion to mainly look for x/2, x/4, and x/16 as far as I remember.

I suggest you two align a bit to avoid duplicate work.

@christianlupus
Copy link
Collaborator Author

Ahh and one more point: I want to push out a release tomorrow. On Saturday there is a NC server release and we need a new cookbook version ready as otherwise these will complain.

I am okay with delaying this until after the release. Just be warned that I am not going to be able to do much about this in the next few hours. 😉

@Weissnix4711
Copy link
Contributor

Yeah ideally we should probably use the same regex, but the easy solution is this:

diff --git a/src/js/yieldCalculator.js b/src/js/yieldCalculator.js
index bfe2da97..47bf75d3 100644
--- a/src/js/yieldCalculator.js
+++ b/src/js/yieldCalculator.js
@@ -11,7 +11,7 @@ function isValidIngredientSyntax(ingredient) {
         It may optionally have a unit but must be proceeded by a single whitespace and further text.
     */
     const ingredientSyntaxRegExp =
-        /^(?:(?:\d+\s)?(?:\d+\/\d+|\p{No})|\d+(?:\.\d+)?)[a-zA-z]*\s.*$/;
+        /^(?:(?:\d+\s)?(?:\d+\/\d+|\p{No})|\d+(?:\.\d+)?)[a-zA-z]*\s.*$/u;
 
     /*
         The ingredientMultipleSeperatorsRegExp is used to check whether the string contains

I missed the u flag.

@christianlupus
Copy link
Collaborator Author

Feel free to give it a go from my side. Ideally there is some error handling involved if we update the regex (for whatever reason), this is not breaking again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Frontend Issue or PR related to the frontend code good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants