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

Add yield calculator #1573

Merged
merged 8 commits into from Apr 9, 2023
Merged

Add yield calculator #1573

merged 8 commits into from Apr 9, 2023

Conversation

j0hannesr0th
Copy link
Contributor

@j0hannesr0th j0hannesr0th commented Mar 26, 2023

This pull request adds a client-side yield calculator to the recipe detail page of every recipe and fixes #116

@christianlupus I wasn't able to find "how to add translations" in the docu. It's my first contribution to a Nextcloud project and I only found a link to transifex. Can you assist/guide me here please?

The function (without translation) is ready:
20230326_215841

Screenshots of the mobile version:
grafik

@j0hannesr0th j0hannesr0th changed the title WIP: Add yield calculator | fixes #116 WIP: Add yield calculator Mar 26, 2023
@j0hannesr0th j0hannesr0th marked this pull request as draft March 26, 2023 20:13
@j0hannesr0th j0hannesr0th changed the title WIP: Add yield calculator Add yield calculator Mar 26, 2023
@christianlupus
Copy link
Collaborator

With respect to the translation issue, you are referring to the warning message, am I right?

In general, you simply use something like

t('cookbook', 'Here comes the text to translate.')

The important part is that everything is on one line (a bug in the implementation of the translation script). Make sure this in interpreted as JS code (so, it must be in a {{}} or an evaluated property). There should be quite some examples in the code. If you run into problems with it, I can do it as well for you.

Could you please (before things gets overly complicated) amend the latest commit and sign off? This will avoid the DCO test to fail.

@github-actions
Copy link

github-actions bot commented Mar 27, 2023

Test Results

     21 files     959 suites   4m 37s ⏱️
   498 tests    498 ✔️ 0 💤 0
3 486 runs  3 485 ✔️ 1 💤 0

Results for commit f70ab41.

♻️ This comment has been updated with latest results.

Signed-off-by: Johannes Roth <johannes.roth@mrsoft.gmbh>
@j0hannesr0th j0hannesr0th marked this pull request as ready for review March 27, 2023 17:46
@j0hannesr0th
Copy link
Contributor Author

@christianlupus I've added the translation, but I'm not sure if I signed-off correctly. I did not edit the changelog, too.

@j0hannesr0th
Copy link
Contributor Author

@christianlupus the pipeline fails because of code style. If I rund the command npm run prettier-fix the translation are being split into multiple lines. What should I do? More translations with less words per translation?

grafik

Should I edit the changelog?

@christianlupus
Copy link
Collaborator

I can tell you that you signed off correctly. Otherwise, the DCO check would fail.

Regarding the failing prettier test, one has to disable the rule for this single line. See for an example in the code:

// prettier-ignore
t('cookbook','Search recipes with this keyword')

Signed-off-by: Johannes Roth <johannes.roth@mrsoft.gmbh>
@j0hannesr0th
Copy link
Contributor Author

@christianlupus ok, codestyle should be fixed. What about the changelog?

@christianlupus
Copy link
Collaborator

Just add an entry to the added section (create one if needed) in the CHANGELOG.md file. That should do it.

Johannes Roth and others added 3 commits April 3, 2023 21:43
Signed-off-by: Johannes Roth <johannes.roth@mrsoft.gmbh>
Signed-off-by: Johannes Roth <johannes.roth@mrsoft.gmbh>
Signed-off-by: j0hannesr0th <36242595+j0hannesr0th@users.noreply.github.com>
@j0hannesr0th
Copy link
Contributor Author

@christianlupus all checks ran successful now. Can you test and merge it please that no merge conflicts must be resolved again?

Thanks!

@christianlupus
Copy link
Collaborator

I already had a quick glance. I wanted to test manually a few more cases and did find one minor thing so far. Will fix and merge or come back to you.

@christianlupus
Copy link
Collaborator

I just checked a few additional cases. Now, I found a few issues, that might come up in the close future (or to help enhance the code base). I will nevertheless approve/merge this PR to get the functionality into the app. The fixes can then be applied step by step.

I will open a new bug with all found issues in order to tackle them.

Signed-off-by: Christian Wolf <github@christianwolf.email>
Signed-off-by: Christian Wolf <github@christianwolf.email>
@christianlupus christianlupus merged commit 381beb1 into nextcloud:master Apr 9, 2023
18 checks passed
@GVLLIFESTYLE
Copy link

With which version will this be released?

@christianlupus christianlupus added this to the Release 0.10.3 milestone Jun 21, 2023
@christianlupus
Copy link
Collaborator

This should be in the next release 0.10.3.

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.

Enhancement: Recalculate ingredient quantity (e.g. double it)
3 participants