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

[UX] Remove left-over help text on Maximum image size field #2062

Closed
jenlampton opened this issue Aug 15, 2016 · 18 comments · Fixed by backdrop/backdrop#4523
Closed

[UX] Remove left-over help text on Maximum image size field #2062

jenlampton opened this issue Aug 15, 2016 · 18 comments · Fixed by backdrop/backdrop#4523

Comments

@jenlampton
Copy link
Member

The description text for the Max image size field reads:

The maximum allowed image size expressed as WIDTHxHEIGHT (e.g. 640x480). Leave blank for no restriction. If a larger image is uploaded, it will be resized to reflect the given width and height. Resizing images on upload will cause the loss of EXIF data in the image.

We should remove the first part (The maximum allowed image size expressed as WIDTHxHEIGHT (e.g. 640x480).) because now that we have pretty integer fields that part of the explanation is nolonger necessary.

@Graham-72
Copy link

But I still need the reminder that it is WIDTH x HEIGHT and not HEIGHT x WIDTH!

@jenlampton
Copy link
Member Author

Good point @Graham-72. Maybe that should be part of the label (and not hidden in the description?)

@Graham-72
Copy link

Yes please. 👍

@ghost
Copy link

ghost commented Oct 12, 2020

I think the explanation in the description is fine. I don't think adding 'width x height' to the field label is a good idea.

Any objections to closing this then?

@ghost ghost added the type - task label Oct 12, 2020
@stpaultim
Copy link
Member

stpaultim commented Oct 12, 2020

I think the current text is ok as well, but it does leave room for improvement. I don't feel strongly about this, but if someone were so inclined to write a Pull Request I think it would be an improvement.

I'm adding a screen shot to help folks looking at this issue for the first time. This applies to the maximum image size on the field configuration page (at least, that's what I think it applies to - the text may have changed slightly already).

Image___PR_3367_backdrop_backdrop_testing_site_

@stpaultim
Copy link
Member

As part of a OpenForce2022 I did a PR demo and we created a PR for this issue.

Please, take a look and give us feedback.

@ghost
Copy link

ghost commented Mar 12, 2022

FTR, here's what it looks like with the above PR:

image

@ghost
Copy link

ghost commented Mar 12, 2022

I'd personally prefer something like this instead:

image

And here's a PR that does just that: backdrop/backdrop#3983

  • Removes 'width/height' text from description
  • Adds placeholder text instead
  • Reduces width of number fields to look more natural

@stpaultim
Copy link
Member

stpaultim commented Mar 12, 2022

@BWPanda - excellent improvement. I closed my PR in favor of yours and tested it. I will mark it "Works for Me."

I also marked this a "Milestone Candidate - Bug". It's very close.

OPENFORCE 2022 NOTE:
Sometimes we create a PR that sparks a better idea and a new PR. This is a valuable way to help move the project forward.

@argiepiano
Copy link

argiepiano commented Mar 12, 2022

I do have a question about the latest PR by @BWPanda. When values are input into the fields, you don't know which field is for width and which for height. It may not too be problematic, but I think it's helpful to know at a glance. I kind of prefer the previous screenshot.

@indigoxela
Copy link
Member

indigoxela commented Mar 20, 2022

When values are input into the fields, you don't know which field is for width and which for height.

Yes, that's the culprit with placeholders. With existing values they're pretty worthless. TBH adding back (width x height) also seems better to me.

width-or-height

Another tiny thing: the "size" attribute is not officially supported for number input. It only works in some browsers, but not in all. I know that, because I ran into that myself. 😉

@bugfolder
Copy link

We have two PRs, both of which have (I think) been tested successfully, but provide different approaches:

@argiepiano, @indigoxela, and I prefer explicit. @BWPanda and @stpaultim prefer placeholders. Maybe we could get a little more input to move toward consensus (or at least, a clear majority?).

@olafgrabienski
Copy link

I prefer the label with "width x height". (And in @BWPanda's screenshot I like the reduced width of number fields.)

@jenlampton
Copy link
Member Author

I also prefer explicitly stating it in the description, but can we also add placeholders? :) Best of both worlds?

@bugfolder
Copy link

YAPR (Yet Another Pull Request) backdrop/backdrop#4523. In response to the above, and discussion at the 2023-09-21 dev meeting. Points made there:

  • Placeholders are nice to have, but are not accessible for all, so it's still nice to have the order spelled out in the description text.
  • We don't need an entire sentence for "widht x height," so we can still streamline the wording.

Here's the result of the new PR, first, as the form initially comes up:

image

And then what it looks like with data already filled in (e.g., editing a field):

image

@quicksketch
Copy link
Member

Thanks @bugfolder! This is essentially @BWPanda's PR but with some description text rewording. Looks good! I left an item of feedback, that #size does not have any effect on number fields. It outputs the attribute, but it doesn't affect the display in the browser as far as I know. See #6137, where we actually removed #size from various number fields.

@bugfolder
Copy link

This is essentially @BWPanda's PR but with some description text rewording.

Yes. Thanks for the #size note, I confirmed that it doesn't affect what the browser shows (at least in Mac Safari) and accepted those changes.

@quicksketch
Copy link
Member

I gave backdrop/backdrop#4523 a final review and it looks and works great. Thanks @bugfolder for pushing this to the finish! Lots of folks to thank for this little improvement. backdrop/backdrop@909f50d by @bugfolder, @BWPanda, @indigoxela, @olafgrabienski, @jenlampton, @Graham-72, @stpaultim, @argiepiano, and @quicksketch.

Merged into 1.x and 1.26.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment