Skip to content

[Refactor] Split up RangeSelector.cs file #3822

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Mar 8, 2021
Merged

[Refactor] Split up RangeSelector.cs file #3822

merged 11 commits into from
Mar 8, 2021

Conversation

Rosuavio
Copy link
Contributor

@Rosuavio Rosuavio commented Mar 8, 2021

Fixes #None

PR Type

What kind of change does this PR introduce?

  • Refactoring (no functional changes, no api changes)

What is the current behavior?

What is the new behavior?

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Pull Request has been submitted to the documentation repository instructions. Link:
  • Sample in sample app has been added / updated (for bug fixes / features)
  • New major technical changes in the toolkit have or will be added to the Wiki e.g. build changes, source generators, testing infrastructure, sample creation changes, etc...
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Header has been added to all new source files (run build/UpdateHeaders.bat)
  • Contains NO breaking changes

Other information

@ghost
Copy link

ghost commented Mar 8, 2021

Thanks RosarioPulella for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

@ghost ghost requested review from michael-hawker, azchohfi and Kyaa-dost March 8, 2021 19:51
@Rosuavio Rosuavio changed the title [Rafactor] Spluit up RangeSelector.cs file [Rafactor] Split up RangeSelector.cs file Mar 8, 2021
michael-hawker
michael-hawker requested changes Mar 8, 2021
View reviewed changes
@ghost
Copy link

ghost commented Mar 8, 2021

This PR has been marked as "needs attention 👋" and awaiting a response from the team.

@Rosuavio Rosuavio requested a review from michael-hawker March 8, 2021 20:29
Rosuavio and others added 6 commits March 8, 2021 15:30
@Rosuavio
RangeSelector: Split out Properties file
3c5dc53
@Rosuavio
RangeSelector: Split out Events file
740491d
@Rosuavio
Reorg RangeSelector code
4112efc 
into file based on type of input
@Rosuavio
Move prop changed handlers to prop file
09bd343
@Rosuavio
Fallow the proper 'On___' pattern
df169df
@michael-hawker @Rosuavio
Update RangeMin and RangeMax to RangeStart and RangeEnd
d6fcdfc 
These properties and their descriptions were confusing (and abbreviated) which made it difficult to understand how they differed from the Minimum/Maximum value properties.
This clearly now separates which properties are being selected by the user. Their comments have been updated as well.
It also aligns with the new Range struct field names in .NET: Start/End.

original author: @michael-hawker
@michael-hawker michael-hawker added this to the 7.0 milestone Mar 8, 2021
@ghost ghost added the in progress 🚧 label Mar 8, 2021
This was referenced Mar 8, 2021
Closed
Closed
Closed
@ghost
Copy link

ghost commented Mar 8, 2021

This PR has been marked as "needs attention 👋" and awaiting a response from the team.

@michael-hawker michael-hawker self-requested a review March 8, 2021 20:41
@michael-hawker
Copy link
Member

Cool assuming we pass CI and the Sample works the same, I think we'll be good here! :)

@Kyaa-dost want to take it for a spin too? It should behave the same as the existing control in the 6.1 app.

Basically as long as the StepFrequency is small and within the bounds and a factor of the total range, I think it's fine. I've left the original bug I filed open for 7.1 and we can do more investigations later.

@ghost ghost removed the needs attention 👋 label Mar 8, 2021
Kyaa-dost
Kyaa-dost approved these changes Mar 8, 2021
View reviewed changes
Copy link
Contributor

@Kyaa-dost Kyaa-dost left a comment

Choose a reason for hiding this comment

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

Looks good to me 🚀

@Rosuavio
Copy link
Contributor Author

Rosuavio commented Mar 8, 2021

CI failing, its prob the stylecop stuff. Looking into it.

@Rosuavio
Copy link
Contributor Author

Rosuavio commented Mar 8, 2021

Fixed CI

@michael-hawker michael-hawker changed the title [Rafactor] Split up RangeSelector.cs file [Refactor] Split up RangeSelector.cs file Mar 8, 2021
@michael-hawker michael-hawker merged commit d97513f into CommunityToolkit:master Mar 8, 2021
@ghost ghost added Completed 🔥 and removed in progress 🚧 labels Mar 8, 2021
@Rosuavio Rosuavio deleted the refactor/RangeSelector-SplitFile branch March 9, 2021 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michael-hawker michael-hawker michael-hawker approved these changes

@Kyaa-dost Kyaa-dost Kyaa-dost approved these changes

@azchohfi azchohfi Awaiting requested review from azchohfi

Assignees
No one assigned
Labels
Completed 🔥 controls 🎛️ improvements ✨ introduce breaking changes 💥
Projects
None yet
Milestone
7.0
Development

Successfully merging this pull request may close these issues.
3 participants
@Rosuavio @michael-hawker @Kyaa-dost