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

Chore: expose icon data type #418

Closed
wants to merge 1 commit into from

Conversation

tomasciccola
Copy link
Contributor

@tomasciccola tomasciccola commented Dec 13, 2023

this PR exposes dataType on the iconApi , mostly so we can getById and delete icons (i.e. when importing a new config we delete every field, preset and icon to avoid duplicate stuff).
We could only expose .getByVersionId and delete instead of exposing all of dataType

@tomasciccola tomasciccola requested a review from achou11 December 13, 2023 17:52
@tomasciccola tomasciccola self-assigned this Dec 13, 2023
@tomasciccola tomasciccola mentioned this pull request Dec 13, 2023
3 tasks
Copy link
Member

@achou11 achou11 left a comment

Choose a reason for hiding this comment

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

Can you elaborate on the reason for this? How and where would this be used?

Without any additional context, think it would make more sense to expose specific methods on the IconApi that use the dataType internally, as opposed to exposing the whole dataType.

@tomasciccola tomasciccola requested a review from achou11 December 13, 2023 18:53
@tomasciccola
Copy link
Contributor Author

Can you elaborate on the reason for this? How and where would this be used?

Updated the PR description with more info

@tomasciccola tomasciccola changed the base branch from feat/importConfig to main December 13, 2023 19:03
@tomasciccola tomasciccola changed the base branch from main to feat/importConfig December 13, 2023 19:03
@achou11
Copy link
Member

achou11 commented Dec 13, 2023

hm yeah i think given the context, it would be preferable to expose methods (maybe private symbol ones, depending on where this is being used)

@tomasciccola
Copy link
Contributor Author

I'm closing this PR since I messed stuff with a rebase and this is the least time consuming solution :|, new PR is #421

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