Skip to content

Modify ColorConverters to support two-way bindings#1544

Merged
bijington merged 23 commits intoCommunityToolkit:mainfrom
eduardoagr:main
Feb 16, 2024
Merged

Modify ColorConverters to support two-way bindings#1544
bijington merged 23 commits intoCommunityToolkit:mainfrom
eduardoagr:main

Conversation

@eduardoagr
Copy link
Copy Markdown
Contributor

@eduardoagr eduardoagr commented Nov 20, 2023

Description of Change

Added a new HexToColor converter

Linked Issues

We need a way to convert the RGB color to a Maui color, that Labels Buttons, etc. understand.

  • Fixes #

PR Checklist

Additional information

@TheCodeTraveler

This comment was marked as outdated.

@TheCodeTraveler

This comment was marked as outdated.

@TheCodeTraveler

This comment was marked as outdated.

@bijington
Copy link
Copy Markdown
Contributor

@brminnick there is a discussion where I agreed there is a bug in our converters, they are currently only one way so I suggested we would make them two way.

@TheCodeTraveler
Copy link
Copy Markdown
Collaborator

TheCodeTraveler commented Nov 20, 2023

Apologies. Re-opening.

Please update the PR Description. It is currently just the blank template.

@eduardoagr
Copy link
Copy Markdown
Contributor Author

Apologies. Re-opening.

Please update the PR Description. It is currently just the blank template.

how do I do that, this is my first contibrution

also here is the discution

#1496

@bijington
Copy link
Copy Markdown
Contributor

@eduardoagr thank you for making the effort to provide this implementation. I am hopeful we can make it a more complete offering for developers to use our Color based converters in the toolkit.

I would like to refer back to my point in our original discussion though:

I am happy with the idea of introducing this and in fact we don't need to introduce new converters, I believe we just need to modify our existing converters (e.g. ColorToRgbStringConverter, etc.) to be two way converters. Currently they only convert from target to source which only covers the scenario of getting the Color into the data model and not back to the UI.

I think the changes that we will want to make would be to modify the classes in this file:
https://github.com/CommunityToolkit/Maui/blob/main/src/CommunityToolkit.Maui/Converters/ColorToStringConverter.shared.cs
to inherit from type BaseConverter rather than BaseConverterOneWay as they currently do.

Do you think you would be happy to make those changes instead of introducing a new converter like the initial work does inside this PR?

@eduardoagr
Copy link
Copy Markdown
Contributor Author

Aplogies for the mistake, I updated the template

I also want to update the documentation for converter, how I can do that?

I fout out hat in the documentation, there is only the guide of how to use this is XAML pages, but we can also make it globally

@eduardoagr eduardoagr closed this Nov 20, 2023
@eduardoagr eduardoagr reopened this Nov 20, 2023
@eduardoagr
Copy link
Copy Markdown
Contributor Author

Done

@bijington
Copy link
Copy Markdown
Contributor

bijington commented Nov 21, 2023

@eduardoagr sorry I didn't fully explain everything that was required because I thought the compiler would have provided the guidance. By changing the converters base class from BaseConverterOneWay to BaseConverter you will need to provide an implementation of the ConvertBackTo method. This will need to be the reverse of the ConvertFrom in each converter. This will then result in the converters providing a row way binding support and solving the issue that you originally came across. Looking at the build errors you will also need to implement the DefaultConvertBackReturnValue property on each converter.

As for the documentation we have a separate repository where you can submit a PR to. The repo is at: https://github.com/MicrosoftDocs/CommunityToolkit
I haven't checked the details directly but I think we just need to remove any mention of the converters being one-way in the documentation pages.

@eduardoagr
Copy link
Copy Markdown
Contributor Author

Do you think we can schedule a team call sometime, to have more instrutin I also wanted t talk to you abount including a template that already includes the MVVM Comunity toolkit and Maui Community toolkit

@bijington
Copy link
Copy Markdown
Contributor

bijington commented Nov 22, 2023

Yes I'd be happy to join a call and talk through the changes if that helps. We do also have a Discord server that provides a mechanism for discussing contributions among other things: https://discord.gg/PppdVjWA

As for the template though it's not something we will implement however you can use the extension mentioned in your original discussion on the topic: #1505

@eduardoagr
Copy link
Copy Markdown
Contributor Author

what is yur timezone, so I can program the tams metting

Comment thread src/CommunityToolkit.Maui.UnitTests/Converters/BaseConverterTests.cs Outdated
@bijington bijington changed the title Created a coveter for hex, to Maui Color Modify ColorConverters to support two-way bindings Nov 29, 2023
@ghost ghost added the help wanted This proposal has been approved and is ready to be implemented label Dec 30, 2023
@VladislavAntonyuk
Copy link
Copy Markdown
Contributor

The implementation looks not finished. You changed Rgb converter to BaseConverter, but Rgba remains BaseOneWayConverter.
also can we use Color.FromRgba(value) instead of Color.Parse?

@bijington
Copy link
Copy Markdown
Contributor

I'm going to try and jump in this and get it finished off

@bijington
Copy link
Copy Markdown
Contributor

Color.FromRgba

Color.FromRgba doesn't support converting from the format: RGBA(0,0,0,0)

@bijington
Copy link
Copy Markdown
Contributor

@eduardoagr apologies for the delay in getting this sorted. Would you mind please agreeing to the CLA?

@eduardoagr
Copy link
Copy Markdown
Contributor Author

How do I sign

@eduardoagr
Copy link
Copy Markdown
Contributor Author

@dotnet-policy-service agree

Copy link
Copy Markdown
Contributor Author

@eduardoagr eduardoagr left a comment

Choose a reason for hiding this comment

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

I just sign

@bijington
Copy link
Copy Markdown
Contributor

I just sign

That's great thank you

bijington
bijington previously approved these changes Feb 13, 2024
Copy link
Copy Markdown
Contributor

@bijington bijington left a comment

Choose a reason for hiding this comment

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

Thanks for this @eduardoagr. I'll aim to update our docs shortly

@bijington bijington enabled auto-merge (squash) February 13, 2024 19:45
@bijington bijington disabled auto-merge February 15, 2024 21:27
@bijington bijington enabled auto-merge (squash) February 16, 2024 07:11
@bijington bijington merged commit 26aa5aa into CommunityToolkit:main Feb 16, 2024
@github-actions github-actions Bot locked and limited conversation to collaborators Nov 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

help wanted This proposal has been approved and is ready to be implemented stale The author has not responded in over 30 days

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants