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

[BUU] Style and behaviour updates #12107

Merged
merged 8 commits into from
Feb 5, 2024
Merged

Conversation

dacook
Copy link
Member

@dacook dacook commented Feb 1, 2024

What? Why?

Just some little updates that have popped up. Plus I added a few more..

  1. Addresses padding comments in Slack
    • Product rows ("relaxed") are a total of 56px
    • Variant rows ("condensed") are a total of 40px
      Screenshot 2024-01-25 at 3 01 20 pm
  2. Validation to prevent invalid negative values (per comments)
    Screenshot 2024-01-31 at 4 55 07 pm
  3. Allow direct entry of a number if On Hand field is selected with the keyboard
  4. Fix styling when on hand has a blank value (it's not valid, but can happen, which makes it hard to click on again)
    Now, it just appears blank. We'll probably adjust minimum widths later as part of a broader change.
    Screenshot 2024-02-01 at 11 39 09 am
  5. Fix to prevent "on demand" covering up the next column
    Screenshot 2024-02-01 at 11 57 19 am
  6. Fix column heading alignment
    Columns with inputs should now line up. Well.. almost. The On Hand popout has reduced padding which messes it up a bit, but I won't dwell on that right now.
    Screenshot 2024-02-01 at 12 05 24 pm

What should we test?

As above.

Release notes

Changelog Category (reviewers may add a label for the release notes):

  • User facing changes
  • API changes (V0, V1, DFC or Webhook)
  • Technical changes only
  • Feature toggled

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

Now it's lined up perfectly with the number input.
Using browser validation. I didn't use model validation because the on_hand pseudo-attribute doesn't support it.

But.. it turned out to  not be so simple. Browser validation can't work if the field is hidden, and breaks the javascript. So now I made the javascript smarter, and the end result is more helpful I think.
It's not usually valid, but can still be entered. I wasn't able to fix the positioning of the :after psuedo element without having a child text node. Maybe it's possible to add an empty child text node, but I didn't think it worth getting down to that level..
@dacook dacook added the feature toggled These pull requests' changes are invisible by default and are grouped in release notes label Feb 1, 2024
@dacook dacook self-assigned this Feb 1, 2024
@dacook
Copy link
Member Author

dacook commented Feb 1, 2024

FYI @mariocarabotta I've created these fixes, can you please let me know if you see any problems?

@dacook dacook mentioned this pull request Feb 1, 2024
4 tasks
@mariocarabotta mariocarabotta added pr-staged-au staging.openfoodnetwork.org.au and removed pr-staged-au staging.openfoodnetwork.org.au labels Feb 2, 2024
@mariocarabotta
Copy link
Collaborator

good stuff David!

Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

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

Easy to follow. 👍

Copy link
Collaborator

@rioug rioug left a comment

Choose a reason for hiding this comment

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

Nicely done !

@RachL RachL added the pr-staged-fr staging.coopcircuits.fr label Feb 5, 2024
@RachL
Copy link
Contributor

RachL commented Feb 5, 2024

looking good, merging!

@RachL RachL removed the pr-staged-fr staging.coopcircuits.fr label Feb 5, 2024
@RachL RachL merged commit d4d5449 into openfoodfoundation:master Feb 5, 2024
54 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature toggled These pull requests' changes are invisible by default and are grouped in release notes
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants