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.

Sign up for GitHub

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

Jump to bottom

add image removing on hover #1872

Merged
merged 3 commits into from
Jan 12, 2022
Merged

add image removing on hover #1872

merged 3 commits into from
Jan 12, 2022

Conversation

luka-nextcloud
Copy link
Contributor

@luka-nextcloud luka-nextcloud commented Sep 30, 2021 •
edited by juliusknorr
Loading

Summary

Should be able to remove images inside text when hovering over them.

image

jancborchardt
jancborchardt approved these changes Sep 30, 2021
View reviewed changes
Copy link
Member

@jancborchardt jancborchardt 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 design-wise. As mentioned, as soon as the button component comes, would be best to do it with that (to make sure it works on all sorts of colors of images):
nextcloud-libraries/nextcloud-vue#1808 cc @marcoambrosini

@szaimen
Copy link
Contributor

szaimen commented Sep 30, 2021

How to do that on mobile?

julien-nc
julien-nc requested changes Sep 30, 2021
View reviewed changes
Copy link
Member

@julien-nc julien-nc left a comment

Choose a reason for hiding this comment

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

Very nice!

Built files are included in this repo (in js/). Please include the changed compiled files in the PR.

src/nodes/ImageView.vue Outdated Show resolved Hide resolved
src/nodes/ImageView.vue Outdated Show resolved Hide resolved
src/nodes/ImageView.vue Outdated Show resolved Hide resolved
src/nodes/ImageView.vue Outdated Show resolved Hide resolved
@juliusknorr
Copy link
Member

@jancborchardt Should we just always show the icon for mobile and keyboard navigation?

@jancborchardt
Copy link
Member

@juliushaertl we could, but:

  • @luka-nextcloud mentioned it is difficult to do the keyboard focus on that, and also if you use the keyboard, you can just backspace it away
  • Style-wise it of course looks much better without the icon there all the time. I'd also say that at some point we should remove the file name below for files with preview.

@juliusknorr
Copy link
Member

OK, fine with me then.

@juliusknorr juliusknorr added 2. developing enhancement New feature or request labels Oct 4, 2021
@julien-nc
Copy link
Member

@jancborchardt @juliushaertl How about showing/hiding the icon on click/click-outside? As we would keep the hover events, click events would only be effective in a mobile context.

@luka-nextcloud luka-nextcloud self-assigned this Oct 7, 2021
marcoambrosini
marcoambrosini reviewed Oct 8, 2021
View reviewed changes
Copy link
Member

@marcoambrosini marcoambrosini left a comment

Choose a reason for hiding this comment

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

One thing I just thought about: design-wise, wouldn't it be better if the x button was centered on the upper right corner of the image?

@jancborchardt
Copy link
Member

How about showing/hiding the icon on click/click-outside? As we would keep the hover events, click events would only be effective in a mobile context.

Sounds good @eneiluj

design-wise, wouldn't it be better if the x button was centered on the upper right corner of the image?

Yep @marcoambrosini – actually why isn’t the image shown in full-width anyway @juliushaertl @luka-nextcloud? :)

@juliusknorr
Copy link
Member

  • From call with @jancborchardt images should have the full width if possible (e.g. 4:3 format)

@marcoambrosini
Copy link
Member

From call with @jancborchardt images should have the full width if possible (e.g. 4:3 format)

Not sure if you mean the width of the text or the width of the page of the window. Former case is fine, but with the latter, 4/3 and 16/9 landscape oriented images would not fit in the window on 16:9 displays, so you'll have to scroll on images to reveal their upper and lower parts, which is a bit annoying.

@marcoambrosini
Copy link
Member

Wrong button, sorry 😅

@luka-nextcloud
Copy link
Contributor Author

  • From call with @jancborchardt images should have the full width if possible (e.g. 4:3 format)

@jancborchardt @juliushaertl Do you also want to show a small tiny image as full width?

@juliusknorr
Copy link
Member

Do you also want to show a small tiny image as full width?

No, we should only show it full width if the image size allows, smaller images should not be upscaled.

@luka-nextcloud luka-nextcloud mentioned this pull request Nov 25, 2021
Closed
39 tasks
@azul
Copy link
Contributor

azul commented Jan 5, 2022

I think the image size was fixed with #2002. So my understanding is that this is good to go.

@azul
Copy link
Contributor

azul commented Jan 5, 2022

I rebased everything on current master.
unit tests are failing due to jest complaining about vue syntax in the vue-material-design-icons.

I updated vue-material-design-icons and tried to install @vue/vue2-jest but without any success. I think it might not transform files inside the node modules folder.

@azul
Copy link
Contributor

azul commented Jan 5, 2022

I resolved all conversations that seemed to have been addressed.
There's one open where i am unsure if it was fully addressed.
The PR is still marked as changes requested
@eneiluj could you check if it is okay now and approve?

julien-nc
julien-nc previously approved these changes Jan 6, 2022
View reviewed changes
@julien-nc julien-nc dismissed their stale review January 6, 2022 11:32

It crashes on image deletion

@julien-nc
Copy link
Member

@luka-nextcloud It fails on my side.
getPos() is not found in deleteImage(). Could you have a look?

@julien-nc julien-nc self-requested a review January 6, 2022 12:58
julien-nc
julien-nc requested changes Jan 6, 2022
View reviewed changes
Copy link
Member

@julien-nc julien-nc left a comment

Choose a reason for hiding this comment

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

Can't find getPos in ImageView.

@juliusknorr juliusknorr added this to the Nextcloud 24 milestone Jan 11, 2022
@julien-nc
Copy link
Member

julien-nc commented Jan 11, 2022 •
edited
Loading

@luka-nextcloud @azul I found a fix. What's the best way to push it in your opinion?

  • interactive rebase to remove changes on compiled javascript, rebase on master, add my changes, force push this branch
  • or a simple merge with master with conflict resolution

@juliusknorr
Copy link
Member

interactive rebase to remove changes on compiled javascript, rebase on master, add my changes, force push this branch

👍 for that ;)

luka-nextcloud and others added 3 commits January 12, 2022 15:11
@luka-nextcloud
add image removing feature
20df1c6 
Signed-off-by: Luka Trovic <luka@nextcloud.com>
@azul
fix: vue transform in jest for material design icons
182d4d1 
* Update vue-jest to the current.
* Update vue-material-design-icons to the latest.
* specify `transformIgnorePatterns` in jest config
  to ensure files in vue-material-design-icons are transformed

By default vue-jest ignores all files in node_modules.
So we need to be a bit more specific.

Signed-off-by: Azul <azul@riseup.net>
declare missing getPos prop in <ImageView>
27f98cb 
Signed-off-by: Julien Veyssier <eneiluj@posteo.net>
@julien-nc julien-nc merged commit c97ff9a into master Jan 12, 2022
@delete-merged-branch delete-merged-branch bot deleted the image-removing-on-hover branch January 12, 2022 16:08
@juliusknorr juliusknorr mentioned this pull request Jan 13, 2022
Closed
39 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcoambrosini marcoambrosini marcoambrosini approved these changes

@jancborchardt jancborchardt jancborchardt approved these changes

@julien-nc julien-nc julien-nc approved these changes

@juliusknorr juliusknorr Awaiting requested review from juliusknorr

Assignees

@luka-nextcloud luka-nextcloud

Labels
3. to review enhancement New feature or request
Projects
None yet
Milestone
Nextcloud 24
Development

Successfully merging this pull request may close these issues.

Options for image like delete and alt text?
7 participants
@luka-nextcloud @szaimen @juliusknorr @jancborchardt @julien-nc @marcoambrosini @azul