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

Unify usage of Icons #2814

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Unify usage of Icons #2814

wants to merge 15 commits into from

Conversation

dala318
Copy link
Contributor

@dala318 dala318 commented Oct 10, 2024

Proposal of implementation for #2813

@dala318 dala318 force-pushed the icons branch 2 times, most recently from 71917d9 to b563f2e Compare October 10, 2024 13:03
@dala318
Copy link
Contributor Author

dala318 commented Oct 10, 2024

For now don't want to spend a whole afternoon to copy-pasting icon usage in all services, specially if it's not something you want implemented or in a different way, but fixed the first in alphabetical order as an example for the rest.

@dala318 dala318 marked this pull request as ready for review October 10, 2024 13:13
@5ila5
Copy link
Collaborator

5ila5 commented Oct 12, 2024

I generally do like this. It does not have a very high priority, but having an ENUM for the waste types is quite nice

Note to also update https://github.com/mampfes/hacs_waste_collection_schedule/blob/master/doc/contributing_source.md

I thought about implementing this/or some fallback in the Collection class. The advantage would be that ICS sources would profit from this, as they currently only get the default ICON and you have to set your own if you want to change it. But I don't think this is a good fit as names vary drastically between sources (languages/bin colors/ waste types / bin size ....).

I also thought about implementing a default ICON_MAP to import into sources, but it has basically the same problems/limitations as above

vinzd pushed a commit to vinzd/hacs_waste_collection_schedule that referenced this pull request Oct 12, 2024
- Drop inheritance in favor of EXTRA_INFO, thanks @5ila5
- Align icons on mampfes#2814
- Add more instances: Châteauroux, Saint Quentin en Yvelines, Saumur,
  Versailles
5ila5 added a commit that referenced this pull request Oct 12, 2024
…ances (#2824)

* Add a generic source for publidata based French areas + specific instances

Support for schedules provided by [Publidata](https://www.publidata.io/fr/).

They operate the schedules of at least the following communities:

- GPSEO
- Orléans Métropole
- Tours métropole

A specific source is provided for GPSEO, Orléans Métropole and Tours Métropole.

The reason going for both the generic and specic approaches is to both
improve discoverability of the integration for inhabitants from the
three mentioned areas, and still leave the possibility to some more
adventurous users from other, covered by publidata places, to benefit
from it.

* fix remaining copy-pastingg traces

* Fix & improvements after review

- Drop inheritance in favor of EXTRA_INFO, thanks @5ila5
- Align icons on #2814
- Add more instances: Châteauroux, Saint Quentin en Yvelines, Saumur,
  Versailles

* fix variable name

* Update publidata_fr.md after previous changes

* reformatting + ./update_docu_links.py

---------

Co-authored-by: Vincent Desprez <vincent.desprez@qonto.com>
Co-authored-by: 5ila5 <5ila5@users.noreply.github.com>
@dala318
Copy link
Contributor Author

dala318 commented Oct 14, 2024

Yeah, my idea with this was not to automate icon assignment but only to select one for every basic type and then in every service promote selecting from those when the actual translation is done.

@dala318 dala318 force-pushed the icons branch 2 times, most recently from 7b0b96d to 68f4a91 Compare October 28, 2024 07:37
@dala318
Copy link
Contributor Author

dala318 commented Oct 28, 2024

@dala318
Copy link
Contributor Author

dala318 commented Oct 28, 2024

The fix_icon_use.sh script may only be needed once, so could remove it once the majority of sources have been fixed.

# Build a git-exclude and git-include strings
grep_exclude=""
grep_include=""
for excluded in "${whitelist[@]}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This loop (and above list of excluded files) could be replaced by adding a specific string or in each source file that shall be excluded by check and then replace the loop with the following:

for excluded in $(git grep --name "CUSTOM_ICON_WHITELIST = True" -- $checkpath); do
    base=$(basename $excluded)
    grep_exclude="${grep_exclude} :(exclude)*${base}"
    grep_include="${grep_include} ${checkpath}${base}"
done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants