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

Do not trim whitespace from quantity field item content #820

Merged
merged 1 commit into from Mar 30, 2024

Conversation

mstenta
Copy link
Member

@mstenta mstenta commented Mar 29, 2024

This fixes #817.

I'm not sure if it's the "best" solution, but it was very simple, so I'm throwing it at the wall to see if it sticks. :-)

Curious your thoughts @paul121 since you were the original author of these lines. Were they copied from a core template?

@mstenta
Copy link
Member Author

mstenta commented Mar 29, 2024

Before/after screenshots:

Screenshot from 2024-03-29 14-29-18

Screenshot from 2024-03-29 14-29-29

Copy link
Member

@paul121 paul121 left a comment

Choose a reason for hiding this comment

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

Interesting, this twig syntax is new to me. I believe I did copy this from somewhere in core.

I guess it works... but agree doesn't seem "best". What would it look like to clearly add a space within this loop? Add a "prefix" whenever it is not the 0th item?

I kind of like just using spaces but see how commas could be preferred

@mstenta
Copy link
Member Author

mstenta commented Mar 29, 2024

Interesting, this twig syntax is new to me. I believe I did copy this from somewhere in core.

Me too. I almost didn't notice it at first, and then had to look up what it meant.

What would it look like to clearly add a space within this loop? Add a "prefix" whenever it is not the 0th item?

I'm not sure if there's any benefit to that over just allowing whitespace like this? 🤔

I kind of like just using spaces but see how commas could be preferred

I thought about figuring out how to use commas, but this begs the question: do we use commas in other places?

All of those potential next steps vs this simple fix makes me wonder if we should just commit this to fix the "bug" and worry about "improvements" separately (and maybe more broadly).

Copy link
Member

@paul121 paul121 left a comment

Choose a reason for hiding this comment

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

All of those potential next steps vs this simple fix makes me wonder if we should just commit this to fix the "bug" and worry about "improvements" separately (and maybe more broadly).

Sounds good to me

@mstenta mstenta force-pushed the 3.x-fix-material-type-spacing branch from 2cf9ae3 to 5d24c2d Compare March 30, 2024 20:21
@mstenta mstenta merged commit 5d24c2d into farmOS:3.x Mar 30, 2024
2 checks passed
@mstenta mstenta deleted the 3.x-fix-material-type-spacing branch March 30, 2024 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

no commas in list of multiple materials, material quantity
2 participants