-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Conversation
… other components. Removed unused images.
…rs because they're similar except unicode looks better and we have more control over it using CSS.
@redmunds the sidebar selection sliding animation will be tricky because of the way it's built. There's two separate pieces, the selection and the triangle. |
…hit area is not apparent.
@redmunds I made another change based on @ingorichter's feedback. The current hit area for Find -in-Files pagination icons are really smaller and they have a weird shape (6px by 18px). I've made it 18px by 18px and added subtle hover state so that the hit area is obvious. |
@redmunds nice catch. I've stopped the nested dialog from flashing. |
} | ||
.last-backdrop { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You shouldn't remove this class, and the opacity stuff. Is there so that we don't end up viewing a backdrop for each modal dialog, and just see the one from the last one. If you can see all of them, the bg color gets less opaque with every dialog, making it darker. You can see how it looks by opening the extension manager and then install from url.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this on purpose. Ideally we shouldn't use too many nested dialogs. Having two is dialogs is kind of pushing it and since the second backdrop covers the first dialog it's fine not hiding the first backdrop. Hope that makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know, but even with 2 it looks better with just 1 backdrop. Maybe is better if we fix the classes in the Dialogs widget.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could give a extra class to the first backdrop created and animate only that one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kinda like the multiple backdrops because it also darkens the parent modal dialog.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the darkened look as well because it increases the depth when the second dialog appears.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well is possible to remove the flickering by adding the animation to the first backdrop only.
Anyway, if we are going to remove this class, we should stop adding them to the Dialogs too, by removing the code that does it in src/widgets/Dialogs.js
. https://github.com/adobe/brackets/blob/master/src/widgets/Dialogs.js#L298 and
https://github.com/adobe/brackets/blob/master/src/widgets/Dialogs.js#L322 and https://github.com/adobe/brackets/blob/master/src/widgets/Dialogs.js#L336
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. I’ll go ahead and do that. Thanks Tom!
On Mar 18, 2014, at 4:57 PM, Tomás Malbrán <notifications@github.commailto:notifications@github.com> wrote:
In src/styles/brackets_patterns_override.less:
}
-.last-backdrop {
Well is possible to remove the flickering by adding the animation to the first backdrop only.
Anyway, if we are going to remove this class, we should stop adding them to the Dialogs too, by removing the code that does it in src/widgets/Dialogs.js. https://github.com/adobe/brackets/blob/master/src/widgets/Dialogs.js#L298 and
https://github.com/adobe/brackets/blob/master/src/widgets/Dialogs.js#L322 and https://github.com/adobe/brackets/blob/master/src/widgets/Dialogs.js#L336
—
Reply to this email directly or view it on GitHubhttps://github.com//pull/5921/files#r10731960.
@larz0 the enlarged hit area looks good. |
} | ||
.last-backdrop { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TomMalbran @larz0 I thought part of the reason we added this was a z-order fix of some sort... are we sure it's safe to remove now? Looks like PR #4714 or #5085 maybe...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The z-index is added directly to each modal and backdrop element. I added the .last-backdrop
class so that we only show the last backdrop.
@redmunds I've addressed the feedbacks. |
Looks good. Merging. |
What's new: