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: reduce repeated code #176

Conversation

MaxAndreassenRPM
Copy link
Contributor

An idea to reduce the duplication between the bespoke asset panel types.

@MaxAndreassenRPM MaxAndreassenRPM requested a review from a team as a code owner November 30, 2023 21:01
await LoadTemplateRenders();
CreateButtons(avatarTemplates.ToArray(), async (button, asset) =>
{
var downloadHandler = new DownloadHandlerTexture();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We really need to wrap this into some sort of reusable image download request class. I know it was mentioned we had one somewhere but I couldn't find it 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

You can download image by using DownloadTexture extension of WebRequestDispatcher, like this

 var requestDispatcher = new WebRequestDispatcher();
 await requestDispatcher.DownloadTexture("url", CancellationToken.None);

@@ -29,6 +33,23 @@ public ButtonElement CreateButton(string id)
return button;
}

public void CreateButtons<T>(T[] assets, Action<ButtonElement, T> onButtonCreated = default) where T : IAssetData
Copy link
Collaborator

Choose a reason for hiding this comment

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

don't you think that this make it look more complicated (and potentially harder to write simple documentation for) with the use of generics and function passing?
I understand that you wanted to remove some code duplication though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like generics in general, but this could be overengineering, though I don't know any other way to remove duplication right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally, I don't really see it as an issue, because it is a generic in a lower level part of the element? The core documentation for partners would be that they could easily extend this class with their own functions to create their own selector, and they get a helpful callback to edit the button style if they want to.

IMO this is easier to understand and extend than it would be the other way around since there's less uniformity otherwise.

In my mind this strips away the code that the partners don't need care about, but of course they can if they want to.

@MaxAndreassenRPM
Copy link
Contributor Author

Closing this as @HarrisonHough has picked out the useful bits into his own PR 😄

@HarrisonHough HarrisonHough deleted the feature/asset-panel-element-suggestion branch December 22, 2023 06:59
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.

3 participants