Skip to content

Comments

add image removing on hover#1872

Merged
julien-nc merged 3 commits intomasterfrom
image-removing-on-hover
Jan 12, 2022
Merged

add image removing on hover#1872
julien-nc merged 3 commits intomasterfrom
image-removing-on-hover

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.

juliusknorr
juliusknorr reviewed Oct 4, 2021
View reviewed changes
@juliusknorr
Copy link
Member

juliusknorr commented Oct 4, 2021

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

@jancborchardt
Copy link
Member

jancborchardt commented Oct 4, 2021

@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

juliusknorr commented Oct 4, 2021

OK, fine with me then.

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

julien-nc commented Oct 4, 2021

@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 force-pushed the image-removing-on-hover branch from 7c63cb0 to 54eea5a Compare October 6, 2021 19:41
@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

jancborchardt commented Oct 11, 2021

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

juliusknorr commented Oct 11, 2021

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

@marcoambrosini
Copy link
Member

marcoambrosini commented Oct 18, 2021

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

marcoambrosini commented Oct 18, 2021

Wrong button, sorry 😅

@jancborchardt
Copy link
Member

jancborchardt commented Oct 18, 2021

Not sure if you mean the width of the text or the width of the page of the window.

Width of text, like on Medium etc. :)

@luka-nextcloud
Copy link
Contributor Author

luka-nextcloud commented Oct 19, 2021

  • 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

juliusknorr commented Oct 20, 2021

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 azul force-pushed the image-removing-on-hover branch from 31af611 to 3699ad4 Compare January 5, 2022 11:07
@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 force-pushed the image-removing-on-hover branch from 0b5e416 to 45cd4fc Compare January 6, 2022 11:23
@julien-nc julien-nc dismissed their stale review January 6, 2022 11:32

It crashes on image deletion

@julien-nc
Copy link
Member

julien-nc commented Jan 6, 2022

@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

juliusknorr commented Jan 12, 2022

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

👍 for that ;)

@julien-nc julien-nc force-pushed the image-removing-on-hover branch from 45cd4fc to fac9196 Compare January 12, 2022 12:02
@julien-nc julien-nc force-pushed the image-removing-on-hover branch from fac9196 to b341a1b Compare January 12, 2022 12:34
luka-nextcloud and others added 3 commits January 12, 2022 15:11
@luka-nextcloud
add image removing feature
20df1c6 
Signed-off-by: Luka Trovic <[email protected]>
@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 <[email protected]>
declare missing getPos prop in <ImageView>
27f98cb 
Signed-off-by: Julien Veyssier <[email protected]>
@julien-nc julien-nc force-pushed the image-removing-on-hover branch from 06210ca to 27f98cb Compare January 12, 2022 14:13
@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