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

Replace tune icon with filter icon #10056

Merged
merged 1 commit into from
Sep 12, 2024

Conversation

Jerome-Herbinet
Copy link
Member

Fixes: #9942

Check commit.

I don't have a test environment. Test from reviewers need.

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.

The icon component is not registered on the Vue component

@Jerome-Herbinet
Copy link
Member Author

The icon component is not registered on the Vue component

@ChristophWurst this is new for me ; how to do that ? Can you help me ?

@ChristophWurst
Copy link
Member

Delete

import Tune from 'vue-material-design-icons/Tune.vue'
and replace Tune with FilterIcon at

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.

@Jerome-Herbinet Jerome-Herbinet force-pushed the Jerome-Herbinet-replace-tune-icon-with-filter-icon branch from 295a1dc to f4766be Compare August 27, 2024 14:03
@Jerome-Herbinet
Copy link
Member Author

Delete

import Tune from 'vue-material-design-icons/Tune.vue'

and replace Tune with FilterIcon at

@ChristophWurst OK

Copy link

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

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.

Looks good!

Did not test

@Jerome-Herbinet
Copy link
Member Author

@jancborchardt could you test it ?

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.

@Jerome-Herbinet as per my comment before, the icon probably needs to be "FilterVariant".

It should not look like this funnel icon, but rather like this list filter icon.

@Jerome-Herbinet
Copy link
Member Author

@Jerome-Herbinet as per my comment before, the icon probably needs to be "FilterVariant".

It should not look like this funnel icon, but rather like this list filter icon.

OK @jancborchardt ; to make finding icons easier, can you tell me how (and/ore where) you easily browse your entire Nextcloud icon collection to select the right one (AFAIK I can't open files viewed as an image...).

@jancborchardt
Copy link
Member

@Jerome-Herbinet as far as I know, that is https://pictogrammers.com/library/mdi/ – but a frontender would have to help here.

Copy link
Member

@st3iny st3iny left a comment

Choose a reason for hiding this comment

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

Works so far. Just needs the correct icon, as Jan mentioned.

grafik

@Jerome-Herbinet Jerome-Herbinet force-pushed the Jerome-Herbinet-replace-tune-icon-with-filter-icon branch from ad31d76 to 79079fc Compare September 12, 2024 13:28
@ChristophWurst
Copy link
Member

What was force pushed? The icon is still to be replaced, right?

@Jerome-Herbinet
Copy link
Member Author

What was force pushed? The icon is still to be replaced, right?

Yes, forget it, I still have to make discussed changes.

@Jerome-Herbinet
Copy link
Member Author

Please check and test my last commit @jancborchardt @st3iny

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.

Seems good – don’t know about the icon naming conventions but it seems the variables usually don’t have "Icon" appended, so you could call it simply "FilterVariant" instead of "FilterVariantIcon"?

Copy link
Member

@st3iny st3iny left a comment

Choose a reason for hiding this comment

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

Tested and works. Thank you very much :)

Signed-off-by: Jérôme Herbinet <[email protected]>
Signed-off-by: Richard Steinmetz <[email protected]>
@st3iny st3iny force-pushed the Jerome-Herbinet-replace-tune-icon-with-filter-icon branch from 0cb81cb to 71af3f8 Compare September 12, 2024 18:21
@st3iny st3iny marked this pull request as ready for review September 12, 2024 19:00
@st3iny st3iny merged commit ee53cb8 into main Sep 12, 2024
35 checks passed
@st3iny st3iny deleted the Jerome-Herbinet-replace-tune-icon-with-filter-icon branch September 12, 2024 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing design enhancement feedback-requested filter:search skill:frontend Issues and PRs that require JavaScript/Vue/styling development skills
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Confusing search icon (needs to be replaced by a more suggestive one)
4 participants