backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 40 forks source link

Add the complete file path to the manage file form. #4175

Closed jenlampton closed 10 months ago

jenlampton commented 5 years ago

When I'm managing my files, I really want to know the file name (not the human-readable label we already show in the form, but the actual name of the document on disk). I also sometimes wonder where that file is stored, because I'd like to test it directly in my browser. We don't reveal any of this information to the user, and it seems both are a valuable part of managing files.

I'd like to propose the following improvements to the Manage File form:

Screenshot:

Screen Shot 2019-10-30 at 1 55 35 PM

~I went with "File link text" in the screenshot (and first PR) but am open to recommendations on what to call the human-friendly name for a file~

~I went with Description as the label for that field, since that is what we are calling the same thing in other interfaces. See https://github.com/backdrop/backdrop-issues/issues/4178 for better text to use there.~


PR: https://github.com/backdrop/backdrop/pull/2960


Translation updates:

laryn commented 5 years ago

Is what you refer to as "File link text" the same thing as what is called "Description" elsewhere in the UI?

Screen Shot 2019-10-30 at 3 20 54 PM

jenlampton commented 5 years ago

Yes, according to that description text. But I think "Description" is an even worse label for it! I like the description text on the Description field there though.

Maybe we should call this Text for the link to the file

jenlampton commented 5 years ago

@laryn where did you find that UI? I just added a file field to a node and it's not there...

edit: Oh, I found an "enable description field" checkbox... but I didn't check it because I didn't think the file label was a description.... even though we just covered this!

laryn commented 5 years ago

I suppose using what is already used elsewhere makes sense here, and then opening a separate issue to suggest that we change that descriptor throughout, if it's no good?

Separately, this thread is related: https://github.com/backdrop/backdrop-issues/issues/3904

jenlampton commented 5 years ago

I suppose using what is already used elsewhere makes sense here, and then opening a separate issue to suggest that we change that descriptor throughout, if it's no good?

Yep, good idea. Will use Description here now, and I'll add the description text I like too :)

Screen Shot 2019-10-30 at 1 55 35 PM

edit: here's the other issue for deciding what to name it https://github.com/backdrop/backdrop-issues/issues/4178

klonos commented 5 years ago

Thanks @jenlampton πŸ‘ ...I wouldn't mind holding off until we've made a decision in #4178 and got this all in one go ...I think we'll reach to a decision soon πŸ˜‰

klonos commented 5 years ago

...I've left comments in the PR. I love this ❀️ ...just minor tweaks implementation-wise; is all.

klonos commented 4 years ago

Hey @jenlampton ...I wanted to git this a go, but the PR sandbox doesn't seem to work, and there seem to be conflicts (after merging of #4178 I guess). Can you please resolve the conflict, and make sure that the sandbox works? ...if not, then perhaps try closing the PR and reopening it πŸ™‚

It Crowd - Try turning it off and on again

herbdool commented 2 years ago

I think this looks good. I resolved the conflicts on the PR. And reviewed/tested.

laryn commented 2 years ago

Did this one get closed without merging as well @jenlampton https://github.com/backdrop/backdrop/pull/2960

quicksketch commented 2 years ago

Yeah, @jenlampton I think you need to re-push this branch? Also I think there's a missing check_plain(): https://github.com/backdrop/backdrop/pull/2960#discussion_r785254206

jenlampton commented 2 years ago

huh. I wonder what happened to all my branches? Anyway - I added the check_plain() and pushed my branch back up.

klonos commented 2 years ago

Some feedback:

klonos commented 2 years ago
  • Since the filename cannot be edited, perhaps we could follow the same UI pattern as the machine name, and display it similarly next to the description field?

Never mind ...just found out that we have a similar UI pattern for the language configuration form:

image

quicksketch commented 2 years ago

@jenlampton Sorry! I think there's one more place that needs a check_plain(): https://github.com/backdrop/backdrop/pull/2960/files#r793279222

jenlampton commented 2 years ago

I've merged your suggested change @quicksketch, this is back to RTBC.

quicksketch commented 2 years ago

There's an accidental revert of some terminology in the PR: https://github.com/backdrop/backdrop/pull/2960#pullrequestreview-951134213

Should the "File URL" be linkified?

I agree with this, seems weird to display a URL but not have it be clickable. However, that introduces a new UX problem where clicking the link may make any in-progress changes get lost. We could adjust the selector for the Backdrop.behaviors.filePreviewLinks behavior though to work both here and on file upload elements (where it's used currently).

quicksketch commented 2 years ago

I filed a PR against @jenlampton's PR at https://github.com/jenlampton/backdrop/pull/15 that makes the URL clickable and reverts the terminology regressions. https://github.com/jenlampton/backdrop/pull/15

stpaultim commented 10 months ago

I'm trying to help some old PR's reach the finish line. To get a sandbox this one needed to be rebased. I simply created a new PR that is up to date with current code base. https://github.com/backdrop/backdrop/pull/4619

I believe that my current PR does contain the last suggested changes by @quicksketch (someone could help me confirm that).

We can work with mine or @jenlampton can replace her own with mine to move this forward.

This is what this page looked like before.

image

This is what it looks like after PR.

image

Needs testing and code review.

stpaultim commented 10 months ago

There is one test failure right now that looks like it is probably legit, I'm not sure I understand what is going on there. Can anyone help me with that?

olafgrabienski commented 10 months ago

@stpaultim I've tested the new PR with a PDF file, and it looks good to me: The vertical tab looks like in your screenshot, the File URL is clickable and opens in a new small window.

quicksketch commented 10 months ago

The changed code looks great, thanks @herbdool for coming in and fixing the use of .on() in the JavaScript.

There's a legit failing test across all versions of PHP:

Status    Group      Filename          Line    Function                            
------------------------------------------------------------------------------------------------------------------------
Fail      Other      file.test         1390    testNodeDisplay()                                                       
    Default formatter displaying correctly on full node view.

Looks like it just needs to be updated for the new markup created by this PR.

herbdool commented 10 months ago

I fixed file.test and put it back to RTBC since the test was the only thing holding back.

quicksketch commented 10 months ago

Thanks @herbdool! I merged https://github.com/backdrop/backdrop/pull/4619 into 1.x and this is the first issue in the new 1.27.x branch to be fixed. Thanks @jenlampton, @stpaultim, @laryn, @olafgrabienski, and @klonos!