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

feat: range, add reference point to start range slider at #24348 #24399

Closed

Conversation

sachingarg05
Copy link
Contributor

@sachingarg05 sachingarg05 commented Dec 14, 2021

Added property barActiveStart. bar-active is shown between barActiveStart and knob for single knob range-bar. By default barActiveStart is set to min (Minimum integer value of the range).

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Build (npm run build) was run locally and any changes were pushed
  • Lint (npm run lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the new behavior?

Details were discussed in issue Number: #24348

Does this introduce a breaking change?

  • Yes
  • No

Other information

md:
image
ios:
image

…24348

Added property barActiveStart. bar-active is shown between `barActiveStart` and knob for single knob range-bar. By default `barActiveStart` is set to `min` (Minimum integer value of the range).
@sachingarg05 sachingarg05 requested a review from a team as a code owner December 14, 2021 15:14
@github-actions github-actions bot added package: angular @ionic/angular package package: core @ionic/core package package: vue @ionic/vue package labels Dec 14, 2021
Copy link
Contributor

@sean-perkins sean-perkins left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for this PR 👍

Could you add an e2e test here: https://github.com/ionic-team/ionic-framework/tree/main/core/src/components/range/test (create a new folder) for this feature?

You can look at the other e2e.ts files for an example. We are just looking to have an example of this feature and to capture any visual regressions with this feature moving forward.

@sachingarg05
Copy link
Contributor Author

Added a test in core/src/components/range/test/bar-active

md:
image

ios:
image

@sachingarg05
Copy link
Contributor Author

Is this pull request OK for merging or does it need any changes?

@sachingarg05
Copy link
Contributor Author

I was hoping to see this merged for 6.1.x release. Is there anything I can help with to get this merged?

@liamdebeasi
Copy link
Contributor

The feature did not make it into the 6.1.0 release, but it is planned for a future release. We are scheduling time for a code review. A team member will leave feedback on this PR when they have it.

@sachingarg05
Copy link
Contributor Author

Thanks, looking forward to it.

@sachingarg05
Copy link
Contributor Author

Can someone from Ionic team please take a look at this? Really looking forward to see this get merged.

@liamdebeasi
Copy link
Contributor

Hey there,

We plan on looking at this soon. For some additional context, feature PRs typically take longer to merge than bug fix PRs. The reason for this is all features need to go through a design process before any engineering work can begin.

A team member will determine the scope, requirements, proposed architecture, etc. From there, the team will review and refine the proposed design. This is the step we are currently on.

Once the design document has been approved by the team, engineering work begins. A team member will either work with you on any changes, or they will create a new PR and cherry pick certain commits from your PR. Either way, you will receive author credit if the feature lands in the codebase.

@sachingarg05
Copy link
Contributor Author

Thanks for the update. Will be happy to help with any further changes that might be needed to get this merged.

@sean-perkins
Copy link
Contributor

@sachingarg05 thank you so much for this PR contribution.

I'm going to close this in favor of #25598. I will give you co-authored-by credit with that PR is reviewed & merged. We are planning on having this included in the 6.2.0 release.

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: angular @ionic/angular package package: core @ionic/core package package: vue @ionic/vue package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants