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

Fix icon-class, re-add default-icon support for Actions #3017

Merged
merged 1 commit into from
Aug 12, 2022

Conversation

raimund-schluessler
Copy link
Contributor

@raimund-schluessler raimund-schluessler commented Aug 12, 2022

This brings back the support for a default-icon and fixes the support of icon for the Actions component.
Follow-up to #3006, fixes #3016.

Please test this well, I only checked in in the docs.

@raimund-schluessler raimund-schluessler force-pushed the fix/noid/actions-default-icon-class branch from a313166 to 9d29c3c Compare August 12, 2022 08:22
@raimund-schluessler raimund-schluessler added the regression Regression of a previous working feature label Aug 12, 2022
@raimund-schluessler raimund-schluessler removed the request for review from nickvergessen August 12, 2022 08:26
Copy link
Contributor

@CarlSchwan CarlSchwan left a comment

Choose a reason for hiding this comment

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

Probably make sense to at some point have a slot for material icon since we are migrating to that slowly in other places.

But definitively not blocking or urgent, let's first get back default-icon so we can port our apps to Nextcloud Vue 6 :)

Thanks!

@raimund-schluessler
Copy link
Contributor Author

Probably make sense to at some point have a slot for material icon since we are migrating to that slowly in other places.

But definitively not blocking or urgent, let's first get back default-icon so we can port our apps to Nextcloud Vue 6 :)

Thanks!

The component has a slot already, so you can start migrating immediately 😉
That was actually the reason why I didn't bother to re-implement the icon class support, because one can do everything with material design icons just as well.

Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍

@PVince81 PVince81 merged commit 345c46e into master Aug 12, 2022
@PVince81 PVince81 deleted the fix/noid/actions-default-icon-class branch August 12, 2022 08:59
PVince81 added a commit that referenced this pull request Aug 12, 2022
Signed-off-by: Vincent Petry <[email protected]>
@PVince81 PVince81 mentioned this pull request Aug 12, 2022
2 tasks
@PVince81
Copy link
Contributor

PVince81 commented Aug 12, 2022

I did some migrations already back then on Talk and remember that it wasn't always straightforward because you had to find a matching MD icon and sometimes styles started to go crazy. This is why I wouldn't want to rush removing the icon attribute support.

@nickvergessen
Copy link
Contributor

I did some migrations already back then on Talk and remember that it wasn't always straightforward because you had to find a matching MD icon and sometimes styles started to go crazy. This is why I wouldn't want to rush removing the icon attribute support.

Last week I replaced a lot of icons and buttons in Talk, it was mostly all pretty straight forward

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug Something isn't working feature: actions Related to the actions components regression Regression of a previous working feature technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

icon-class support in Actions component broken
4 participants