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

Implements new attachments UI #7676

Merged
merged 3 commits into from Mar 15, 2023

Conversation

Sesquipedalian
Copy link
Member

@Sesquipedalian Sesquipedalian commented Jan 18, 2023

As requested by @sbulen for 2.1.4, here is a new, more streamlined UI for the attachments system.

  1. The drop zone is now always visible directly below the editor. This is very similar to the way attachments are displayed when reading a topic, so it should be much more intuitive.
  2. Attached files are now displayed when editing in a way that is similar to how they are displayed when reading a topic. Again, this should be more intuitive for users.
  3. Uploads begin automatically, rather than requiring the user to click an "UPLOAD" button before sending the file to the server. One of the frequent complaints from users has been that the two-step process of dragging a file into the UI and then having to click the button to actually upload it was both confusing and unnecessary.
  4. The "DELETE" and "INSERT" buttons have been streamlined and simplified. They now appear as two small icons in the upper right corner of each attachment. When the user clicks the insert icon for an image attachment, a small menu panel is shown with the optional width and height input fields. For non-image attachments, clicking the icon simply inserts the [attach] BBC immediately.
  5. Since there is no "ADD FILES" button any more, I changed $txt['attach_drop_zone'] to reflect the new reality.
  6. To accommodate the attachment UI changes, the "Reason for editing" field has been turned into a proper posting field in the post header area. This should probably have been done anyway.
  7. The basic UI that is shown when JavaScript is disabled has also been improved in order to make it as similar as possible to the full UI.
  8. I also found and fixed a bug in the editor where the [attach] BBC would get turned into a [url] BBC for non-image attachments. In 2.1.3, this bug is very minor, since the resulting links still work, but it started causing bigger problems when I made these changes, so I had to fix it.

Because changes to theme templates in a patch release have a high likelihood of causing problems for existing custom themes, I took extra precautions in order to avoid breaking them. In particular, the changes to smf_fileUpload.js and Post.php include steps to ensure that they remain fully backward compatible with theme files from SMF 2.1.3 and earlier. Those extra tests and fallbacks can safely be removed in SMF 2.2.

UI when there are no attachments:
Screenshot 2023-02-26 at 3 04 57 PM

Demonstrating all UI changes (note: mouse cursor isn't visible in the screenshot, but it is dragging the RIS file):
Screenshot 2023-02-26 at 3 06 07 PM

Improved UI when JavaScript is disabled:
Screenshot 2023-01-17 at 7 30 25 PM

This PR also fixes #7682.

Signed-off-by: Jon Stovell <jonstovell@gmail.com>
@DiegoAndresCortes
Copy link
Contributor

DiegoAndresCortes commented Feb 4, 2023

  • Could you remove the commented lines on the css?

  • There's also some padding that gives the illusion of having more area to drop images:

Screenshot_57

It should have full height.
  • Additionally I have a suggestion, could you keep the upload bar always above the other items? This way users don't have to "find" where the upload block went.
    e.g.

Screenshot_56

Screenshot_55

  • Something like this could be a bit better?
    I enabled the flex area so it's more visually understandable. The whole area can be used to drag items, but I kept the buttons with a higher z-index so you can still delete them or insert them.

Screenshot_58

Perhaps you have another idea on how to improve this based on my usable concept.

Overall I like it, it would be a great step up.

@jdarwood007
Copy link
Member

Also, to expand this. Is it possible to make it so dragging the thumbnail to the post would do the inline attachment as well?

@sbulen sbulen added this to the 2.1.4 milestone Feb 12, 2023
@Sesquipedalian
Copy link
Member Author

Sesquipedalian commented Feb 16, 2023

@DiegoAndresCortes: Part of my overall goal has been to make this new layout as similar as possible to the layout used to show attachments in the topic display. That's why I intentionally did not enable a full flex layout in the section showing the current attachments.

I see what you mean about wanting to keep the drop zone area as large and as consistent as possible. However, I think I might be able to do you one better in that regard. I will need to experiment with it to see if it will work, but I might just designate the entire attachments container div as the drop zone area. Then padding, positioning of the instructional text, etc., will not matter.

@jdarwood007: All things are possible. But that kind of drag and drop interaction between the attachments area and the editor would probably require a significant amount of new JavaScript. Implementing it would also unequivocally cross over the line from revamping an existing feature to introducing an entirely new feature. I'm not in charge of anything anymore, but unless @sbulen has changed the SMF development policies, that means adding something like that would need to wait for SMF 2.2.

@Sesquipedalian
Copy link
Member Author

Sesquipedalian commented Feb 26, 2023

I have now updated this PR according to the previous comments and discussion.

  1. Instructional text has been changed to 'Click or drag files here to attach them.'
  2. Instructional text always appears above the attachment previews.
  3. The drop zone is now the entire attachments display area, rather than just a part of it.
  4. When the user drags a file into the window, a dotted border appears around the entire drop zone to help indicate where the drop zone area is.
  5. Both the div containing the instructional text, as well any blank space to the right of the attachment previews, can be clicked to open a file selection dialog.
  6. The mouse cursor changes to the "pointer" style when hovering over the clickable elements.
  7. Some minor layout issues have been fixed.

I have updated the screenshots in the original post to reflect these changes.

@Sesquipedalian Sesquipedalian force-pushed the attachments_ui branch 2 times, most recently from e78c50e to 1022bb1 Compare February 27, 2023 01:28
@sbulen
Copy link
Contributor

sbulen commented Feb 28, 2023

I'll be testing this over the next day or two.

@sbulen
Copy link
Contributor

sbulen commented Mar 3, 2023

This is excellent. So far, I've mainly tested in Firefox, & am going to start testing in other browsers.

The only nit I have is that I sometimes see doubling up of the INSERT controls on some custom themes, like this:
image

Both sets work.

I am not sure if you can do something about this? This issue goes away if I bring over the index.css changes from this PR into the custom theme.

If you can do something, that would help. If not, we will need to make an announcement upon launch, & reach out to the theme developers.

@sbulen
Copy link
Contributor

sbulen commented Mar 3, 2023

The two themes I've tested with that I see the above issue on are Hextech & Starflight.

@sbulen
Copy link
Contributor

sbulen commented Mar 3, 2023

This is so much cleaner...

@Sesquipedalian
Copy link
Member Author

The only nit I have is that I sometimes see doubling up of the INSERT controls on some custom themes, like this:

image

Both sets work.

I am not sure if you can do something about this? This issue goes away if I bring over the index.css changes from this PR into the custom theme.

If you can do something, that would help.

I probably can. It will most likely require changing some class names on the affected elements, which will in turn require more changes in the JavaScript, but it ought to be possible. I'll see what I can do as soon as I have a few spare minutes for it.

@Sesquipedalian
Copy link
Member Author

One question, @sbulen: Do the themes you saw problems with use their own versions of Post.template.php, or do they just fall through to the default theme's version?

@sbulen
Copy link
Contributor

sbulen commented Mar 3, 2023

I know that Starflight uses the default version. No computer access to look at the other atm...

That's why this is interesting!

@Sesquipedalian
Copy link
Member Author

Sesquipedalian commented Mar 3, 2023

Thanks. That's helpful information.

It is interesting, I suppose, particularly if "interesting" is defined as "an example of why we normally avoid changes to theme files in patch releases," or "a pain in Sesquipedalian's butt." But c'est la vie. 😉

Signed-off-by: Jon Stovell <jonstovell@gmail.com>
@Sesquipedalian
Copy link
Member Author

Sesquipedalian commented Mar 4, 2023

Try this version, @sbulen.

  1. I have moved all the changes I had previously made in index.css into a new CSS file named attachments.css. This new file is loaded via loadCssFile() in Post.php and Display.php.
  2. I made a few tweaks to the JavaScript to accommodate these changes.

Putting the CSS changes into a new file should solve the inconsistency issues for custom themes that use the default Post.template.php but their own index.css file (which is the case for most custom themes). This approach seems to work for the Starflight theme, at any rate.

Signed-off-by: Jon Stovell <jonstovell@gmail.com>
@Sesquipedalian
Copy link
Member Author

Latest commit fixes #7682.

Copy link
Member

@jdarwood007 jdarwood007 left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@sbulen
Copy link
Contributor

sbulen commented Mar 15, 2023

Testing looks good in all browsers I've tried. Even with alt themes.

Copy link
Contributor

@sbulen sbulen left a comment

Choose a reason for hiding this comment

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

LGTM

@sbulen sbulen merged commit 76093f1 into SimpleMachines:release-2.1 Mar 15, 2023
@pr-triage pr-triage bot added the PR: merged label Mar 15, 2023
@Sesquipedalian Sesquipedalian deleted the attachments_ui branch March 15, 2023 06:11
@live627
Copy link
Contributor

live627 commented Mar 27, 2023

In particular, the changes to smf_fileUpload.js and Post.php include steps to ensure that they remain fully backward compatible with theme files from SMF 2.1.3 and earlier. Those extra tests and fallbacks can safely be removed in SMF 2.2.

Should this go into a new issue so that it isn't forgotten?

@Sesquipedalian
Copy link
Member Author

I suppose so.

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

Successfully merging this pull request may close these issues.

Unlimited attachments causes false warnings to user
5 participants