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

[Accepted with Revisions] SDL 0283 - Add Scrollable Message and Slider to Screen Manager #949

Closed
theresalech opened this issue Feb 26, 2020 · 8 comments

Comments

@theresalech
Copy link
Contributor

theresalech commented Feb 26, 2020

Hello SDL community,

The review of "SDL 0283 - Add Scrollable Message and Slider to Screen Manager" begins now and runs through March 17, 2020. A previous review of this proposal took place February 26 - March 3, 2020. The proposal is available here:

https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0283-screen-manager-scrollable-message-and-slider.md

Reviews are an important part of the SDL evolution process. All reviews should be sent to the associated Github issue at:

#949

What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of SDL. When writing your review, here are some questions you might want to answer in your review:

  • Is the problem being addressed significant enough to warrant a change to SDL?
  • Does this proposal fit well with the feel and direction of SDL?
  • If you have used competitors with a similar feature, how do you feel that this proposal compares to those?
  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
    Please state explicitly whether you believe that the proposal should be accepted into SDL.

More information about the SDL evolution process is available at

https://github.com/smartdevicelink/sdl_evolution/blob/master/process.md

Thank you,
Theresa Lech

Program Manager - Livio
[email protected]

@joeygrover
Copy link
Member

While overall this proposal seems straightforward and helpful enough to accept, the proposal itself is hard to distinguish from the review in #948 of SDL 0282 Screen Manager Alert Manager. A lot of sections seem copy and paste, eg paragraph in Proposed solution and Potential Downside section. Is this proposal dependant on SDL 0282? If so, I don't think we should review this proposal until 0282 is reviewed and approved.

@joeljfischer
Copy link
Contributor

I apologize, I didn't remember that I had made this proposal an appending of SDL-0282's alert manager. It is dependent on SDL-0282.

@theresalech theresalech changed the title [In Review] SDL 0283 - Add Scrollable Message and Slider to Screen Manager [Deferred] SDL 0283 - Add Scrollable Message and Slider to Screen Manager Mar 4, 2020
@theresalech
Copy link
Contributor Author

theresalech commented Mar 4, 2020

Per the author's request, the Steering Committee voted to defer this proposal to allow the author to consider any impact the accepted revisions to SDL 0282 might have on this proposal and request changes as needed.

@smartdevicelink smartdevicelink locked and limited conversation to collaborators Mar 4, 2020
@theresalech theresalech changed the title [Deferred] SDL 0283 - Add Scrollable Message and Slider to Screen Manager [In Review] SDL 0283 - Add Scrollable Message and Slider to Screen Manager Mar 11, 2020
@theresalech
Copy link
Contributor Author

It was determined that the accepted revisions to SDL 0282 did not require any changes to this proposal, so this proposal is now back in review. The review will take place until 2020-03-17.

@smartdevicelink smartdevicelink unlocked this conversation Mar 11, 2020
@joeljfischer
Copy link
Contributor

I have noticed a few things in re-reading my proposal:

1.

/**
 This completion handler is used to return the user's selected position or an error if the slider fails. 
*/
typedef void(^SDLScreenManagerSliderCompletionHandler)(NSUInteger selectedPosition, NSError *__nullable error);

selectedPosition should be an NSInteger and the documentation should specify that the selectedPosition will be -1 if there is an error.

2. extern NSTimeInterval SDLScreenManagerDefaultTimeout; // Is 0 in the implementation, used for View class timeouts to activate the defaultTimeout. should be removed, it's not present in the Java Suite implementation and was an earlier idea that I decided not to use.

@theresalech theresalech changed the title [In Review] SDL 0283 - Add Scrollable Message and Slider to Screen Manager [Accepted with Revisions] SDL 0283 - Add Scrollable Message and Slider to Screen Manager Mar 18, 2020
@theresalech
Copy link
Contributor Author

The Steering Committee voted to accept this proposal with the revisions included in this comment from the author.

@theresalech
Copy link
Contributor Author

@joeljfischer please advise when a new PR has been entered to update the proposal to reflect the agreed upon revisions. I'll then merge the PR so the proposal is up to date, and enter issues in respective repositories for implementation. Thanks!

@smartdevicelink smartdevicelink locked as resolved and limited conversation to collaborators Mar 18, 2020
@theresalech
Copy link
Contributor Author

Author has updated PR to reflect agreed upon revisions and implementation issues have been entered:

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants