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

Timob 8019 Modal bug fix. #1685

Merged
merged 6 commits into from Mar 15, 2012
Merged

Timob 8019 Modal bug fix. #1685

merged 6 commits into from Mar 15, 2012

Conversation

nebrius
Copy link
Contributor

@nebrius nebrius commented Mar 15, 2012

No description provided.

@cb1kenobi
Copy link
Contributor

Good catch! Actually, we don't even need to define open() or close() at all in the Window anymore. :)

@nebrius
Copy link
Contributor Author

nebrius commented Mar 15, 2012

Funny you mention that, I was just coming here to note that before you closed it. I think I want to redo this one a bit though and reintroduce modal support by doing a "dimmed" view (like OptionDialog), so let's hold off on this PR until later.


Mobile Web note: All windows are modal from a behavioral standpoint, and all windows can
be translucent or opaque based on the value of `backgroundColor`. Setting the modal value
applies a visual style that is similar to iOS, if the window itself has a height and width
Copy link
Contributor

Choose a reason for hiding this comment

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

What visual style does iOS use? Does this refer to adding a nav bar? Dimming the background?

Something like this might be clearer:

On Mobile Web, windows are always modal, blocking input to underlying windows. If the window does not occupy the full
screen, setting modal to true provides a visual cue by dimming any background windows. If the window occupies the full
screen, modal has no effect.

On Mobile Web and iOS, the modal property has no effect on whether the window is translucent or opaque.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great suggestion!

@arthurevans
Copy link
Contributor

Doc reviewed, suggested rewording.

if (value !== oldValue) {
if (value) {
var parentContainer = this._modalParentContainer = UI.createView();
parentContainer.add(this._modalDimmer = UI.createView({
Copy link
Contributor

Choose a reason for hiding this comment

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

No sense setting this._modalDimmer if it's not used.

@cb1kenobi
Copy link
Contributor

Code reviewed and tested. Request accepted.

@arthurevans
Copy link
Contributor

Accepted!

cb1kenobi added a commit that referenced this pull request Mar 15, 2012
@cb1kenobi cb1kenobi merged commit 4531883 into tidev:master Mar 15, 2012
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.

None yet

3 participants