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

Modify ColorConverters to support two-way bindings #1544

Merged
merged 23 commits into from
Feb 16, 2024

Conversation

eduardoagr
Copy link
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

  • Has a linked Issue, and the Issue has been approved(bug) or Championed (feature/proposal)
  • Has tests (if omitted, state reason in description)
  • Has samples (if omitted, state reason in description)
  • Rebased on top of main at time of PR
  • Changes adhere to coding standard
  • Documentation created or updated: https://github.com/MicrosoftDocs/CommunityToolkit/pulls

Additional information

@brminnick

This comment was marked as outdated.

@brminnick

This comment was marked as outdated.

@brminnick

This comment was marked as outdated.

@brminnick brminnick closed this Nov 20, 2023
@bijington
Copy link
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.

@brminnick
Copy link
Collaborator

brminnick commented Nov 20, 2023

Apologies. Re-opening.

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

@brminnick brminnick reopened this Nov 20, 2023
@eduardoagr
Copy link
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
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
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
Contributor Author

Done

@bijington
Copy link
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
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
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
Contributor Author

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

@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 stale The author has not responded in over 30 days help wanted This proposal has been approved and is ready to be implemented labels Dec 30, 2023
@VladislavAntonyuk
Copy link
Collaborator

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
Contributor

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

@bijington
Copy link
Contributor

Color.FromRgba

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

@bijington
Copy link
Contributor

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

@eduardoagr
Copy link
Contributor Author

How do I sign

@eduardoagr
Copy link
Contributor Author

@dotnet-policy-service agree

Copy link
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
Contributor

I just sign

That's great thank you

bijington
bijington previously approved these changes Feb 13, 2024
Copy link
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 enabled auto-merge (squash) February 16, 2024 07:11
@bijington bijington merged commit 26aa5aa into CommunityToolkit:main Feb 16, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.

None yet

5 participants