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

Change the 'show image' styling #6947

Merged
merged 1 commit into from
Aug 18, 2022
Merged

Change the 'show image' styling #6947

merged 1 commit into from
Aug 18, 2022

Conversation

GretaD
Copy link
Contributor

@GretaD GretaD commented Aug 2, 2022

Fixes #6122

this is how i tried to make it work with the state of actions we have now.
But i agree with Marco's comment here: #6122 (comment)

before
Screenshot from 2022-08-02 15-35-35
Screenshot from 2022-08-02 15-35-22

after
hvcfgh

png

- [ ] Requires nextcloud-libraries/nextcloud-vue#2911

@GretaD
Copy link
Contributor Author

GretaD commented Aug 2, 2022

@jancborchardt and @nimishavijay please have a look at this solution and if you're ok with it

@GretaD GretaD self-assigned this Aug 2, 2022
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.

Regarding the comment from @marcoambrosini, either is fine with me. Indeed using the tertiary button component would be more accurate since it’s not a link which goes somewhere.

@GretaD
Copy link
Contributor Author

GretaD commented Aug 2, 2022

Regarding the comment from @marcoambrosini, either is fine with me. Indeed using the tertiary button component would be more accurate since it’s not a link which goes somewhere.

all the point can be delivered but the icon one. The action must have an icon, it cannot be removed unfortunately, sorry should have commented that before. If we wait for the vue changes, this will be delivered a bit late, if we move on with my solution, it will look like this(plus your comments, but with an icon)

@jancborchardt
Copy link
Member

@GretaD cool, and fine for now with the icon too, but then also use the "image" filetype icon instead of "eye".

@marcoambrosini
Copy link
Member

We can definitely add the option not to have an icon and have the menus triggered by tex-tonly buttons. I think it totally makes sense.

@marcoambrosini
Copy link
Member

...and that will automatically be the case when nextcloud-libraries/nextcloud-vue#2768 happens

@GretaD
Copy link
Contributor Author

GretaD commented Aug 3, 2022

We can definitely add the option not to have an icon and have the menus triggered by tex-tonly buttons. I think it totally makes sense.

i know that, but when it is going to be released? :)

@marcoambrosini
Copy link
Member

pr is there nextcloud-libraries/nextcloud-vue#2911 (comment)
Let's wait for more reviewers since it's a big change. Also you can review @GretaD :)

Once that's we can do a minor release

@GretaD GretaD requested a review from st3iny August 16, 2022 13:57
@GretaD GretaD marked this pull request as ready for review August 16, 2022 13:57
@GretaD GretaD removed the blocked label Aug 16, 2022
@ChristophWurst
Copy link
Member

How is this unblocked when it depends on nextcloud-libraries/nextcloud-vue#2911 and this app still uses v5?

@GretaD GretaD requested review from jancborchardt and removed request for christianlupus August 16, 2022 14:26
@GretaD
Copy link
Contributor Author

GretaD commented Aug 16, 2022

How is this unblocked when it depends on nextcloud/nextcloud-vue#2911 and this app still uses v5?

ah sorry didnt see you added the component on desc. I am not using that component, that was not related to actions without icons, what Marco meant(double checked with him) is that after that is in, we can edit the actions on vue again and add a slot with no icons, but as you i thought that that was supposed to be done on the same pr,

What i did here was hiding the icon with display none.
I dont have time to spend on the component now, so we can either do this or wait till we have time.

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.

Except changing it to plural it looks good to me :)

src/components/MessageHTMLBody.vue Outdated Show resolved Hide resolved
@GretaD
Copy link
Contributor Author

GretaD commented Aug 17, 2022

not ready to merge

@GretaD
Copy link
Contributor Author

GretaD commented Aug 17, 2022

not ready to merge

ready now, please reviewers test it

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Tested, works

@ChristophWurst ChristophWurst merged commit 1a17a3d into main Aug 18, 2022
@ChristophWurst ChristophWurst deleted the improve/show-image branch August 18, 2022 06:35
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.

Improve the "Show images" message and styling
4 participants