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

MDCMigration: Update mat-slider in RangeInputComponent #6659

Merged

Conversation

JamesHollyer
Copy link
Contributor

Motivation for features / changes

This update to the new Angular Materials is a mandate from Angular. Furthermore this RangeInputComponent was kind a a mess with lots of somewhat hacky custom code. The new mat-slider handles all of that for us so this is also a big cleanup.

Technical description of changes

The RangeInputComponent is no longer used for single selection purposes so I removed that option. I also replaced the custom slider with a mat-slider and removed lots of tests that had to do without custom slider implementation.

Screenshots of UI changes (or N/A)

Screenshot 2023-10-20 at 4 54 57 PM Screenshot 2023-10-20 at 4 55 17 PM

Alternate designs / implementations considered (or N/A)

I considered implementing this in the FilterDialog and the RunsTable and then deleting the RangeInputComponent entirely.

@JamesHollyer JamesHollyer changed the title Migrate range selector slider MDCMigration: Update mat-slider in RangeInputComponent Oct 20, 2023
@JamesHollyer JamesHollyer force-pushed the MigrateRangeSelectorSlider branch 8 times, most recently from 0ed5525 to 26a84b1 Compare October 23, 2023 23:42
@JamesHollyer JamesHollyer force-pushed the MigrateRangeSelectorSlider branch from 26a84b1 to 9c1cd68 Compare October 24, 2023 18:34
@@ -43,23 +35,6 @@ enum Position {
*
* Anatomy of the component:
*
* Single slider:
Copy link
Contributor

Choose a reason for hiding this comment

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

👏 The logic was complicated enough to warrant a JSDoc diagram and you got to remove it all

@JamesHollyer JamesHollyer merged commit 2ffb70d into tensorflow:master Oct 25, 2023
13 checks passed
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