Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2133793003:
Unify iron-dropdown CSS (Closed)
|
Created:
4 years, 5 months ago by stevenjb Modified:
4 years, 5 months ago CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUnify iron-dropdown CSS
Moves shared CSS for iron-dropdown to settings_shared_css.html.
Also makes dropdown items actionable.
BUG=621731
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/53b690eb87b6c8786574622243893049d3745bde
Cr-Commit-Position: refs/heads/master@{#404756}
Patch Set 1 #Patch Set 2 : . #
Total comments: 7
Patch Set 3 : Feedback #
Total comments: 2
Patch Set 4 : More feedback #
Messages
Total messages: 24 (5 generated)
Description was changed from ========== Unify iron-dropdown CSS Moves shared CSS for iron-dropdown to settings_shared_css.html. Also makes dropdown items actionable. BUG=621731 ========== to ========== Unify iron-dropdown CSS Moves shared CSS for iron-dropdown to settings_shared_css.html. Also makes dropdown items actionable. BUG=621731 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
stevenjb@chromium.org changed reviewers: + dbeam@chromium.org, dpapad@chromium.org, dschuyler@chromium.org
Sorry if this collides with other cleanup, but need it for Bluetooth.
On 2016/07/08 19:41:04, stevenjb wrote: > Sorry if this collides with other cleanup, but need it for Bluetooth. Also, I compared against all instances of iron-dropdown and none have regressed. In a couple of places we now have the correct 'actionable' pointer.
https://codereview.chromium.org/2133793003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/settings_shared_css.html (right): https://codereview.chromium.org/2133793003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/settings_shared_css.html:63: iron-dropdown .dropdown-content paper-item:hover { Do we want to preserve the hover effect? In dropdown lists we only have focus effect, but not hover (see https://chromium.googlesource.com/chromium/src/+/4d8e2d9c3a4e495f4df013cad9ab...). Also could we get rid of paper-items inside iron-dropdown lists, in favor of divs with a dropdown-item CSS class (see lines 40-56 above). Perhaps that is better suited as a separate cleanup.
https://codereview.chromium.org/2133793003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/settings_shared_css.html (right): https://codereview.chromium.org/2133793003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/settings_shared_css.html:63: iron-dropdown .dropdown-content paper-item:hover { On 2016/07/08 21:01:29, dpapad wrote: > Do we want to preserve the hover effect? In dropdown lists we only have focus > effect, but not hover (see > https://chromium.googlesource.com/chromium/src/+/4d8e2d9c3a4e495f4df013cad9ab...). > > Also could we get rid of paper-items inside iron-dropdown lists, in favor of > divs with a dropdown-item CSS class (see lines 40-56 above). Perhaps that is > better suited as a separate cleanup. I'd prefer to make both changes in a separate cleanup. In particular, the the focus only effect would, imho, be particularly bad for iron-dropdown.
LGTM for unifying CSS. Deferring to @dbeam for the @apply(--settings-actionable) by default part.
lgtm https://codereview.chromium.org/2133793003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.html (left): https://codereview.chromium.org/2133793003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.html:40: visible. --> why are you removing this? https://codereview.chromium.org/2133793003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/settings_shared_css.html (right): https://codereview.chromium.org/2133793003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/settings_shared_css.html:64: @apply(--settings-actionable); I probably would've done this: iron-dropdown .dropdown-content paper-item { @apply(--settings-actionable); } instead of only on :hover, but I suppose this works while --settings-actionable only has cursor: pointer;
Retested iron dropdowns, they look the same. Switching to div will require some work to get the sizing right, but functionally the selectors work fine on divs also. https://codereview.chromium.org/2133793003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.html (left): https://codereview.chromium.org/2133793003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.html:40: visible. --> On 2016/07/08 23:08:08, Dan Beam wrote: > why are you removing this? I looked through the code and found a number of divs with on-tap and no 'actionable' property and no comment. It wasn't clear to me what was special about this one, other than the TODO below which I believe is now resolved. I can restore it if you think it's helpful, but maybe we can clarify it? https://codereview.chromium.org/2133793003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/settings_shared_css.html (right): https://codereview.chromium.org/2133793003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/settings_shared_css.html:64: @apply(--settings-actionable); On 2016/07/08 23:08:08, Dan Beam wrote: > I probably would've done this: > > iron-dropdown .dropdown-content paper-item { > @apply(--settings-actionable); > } > > instead of only on :hover, but I suppose this works while --settings-actionable > only has cursor: pointer; Oh, yeah, that's actually what I had in my head. I will fix this and also use > * instead of paper-item so this will work when we use div instead.
https://codereview.chromium.org/2133793003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.html (left): https://codereview.chromium.org/2133793003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.html:40: visible. --> On 2016/07/08 23:50:05, stevenjb wrote: > On 2016/07/08 23:08:08, Dan Beam wrote: > > why are you removing this? > > I looked through the code and found a number of divs with on-tap and no > 'actionable' property and no comment. It wasn't clear to me what was special > about this one, other than the TODO below which I believe is now resolved. which ones? > > I can restore it if you think it's helpful, but maybe we can clarify it? how would you clarify it?
https://codereview.chromium.org/2133793003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/settings_shared_css.html (right): https://codereview.chromium.org/2133793003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/settings_shared_css.html:63: iron-dropdown .dropdown-content > * { i think paper-item is better than "> *" here because: a) it works even if the <paper-item> is wrapped with another tag, i.e. <div><paper-item> b) separators seem fairly likely in the future (and they probably shouldn't be actionable)
https://cs.chromium.org/search/?q=on-tap+file:%5Esrc/chrome/browser/resources... First two instances don't have 'actionable'. Maybe those are outliers? On Fri, Jul 8, 2016 at 5:09 PM, <dbeam@chromium.org> wrote: > > > https://codereview.chromium.org/2133793003/diff/20001/chrome/browser/resource... > File > > chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.html > (left): > > > https://codereview.chromium.org/2133793003/diff/20001/chrome/browser/resource... > > chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.html:40: > visible. --> > On 2016/07/08 23:50:05, stevenjb wrote: > > On 2016/07/08 23:08:08, Dan Beam wrote: > > > why are you removing this? > > > > I looked through the code and found a number of divs with on-tap and > no > > 'actionable' property and no comment. It wasn't clear to me what was > special > > about this one, other than the TODO below which I believe is now > resolved. > > which ones? > > > > > I can restore it if you think it's helpful, but maybe we can clarify > it? > > how would you clarify it? > > https://codereview.chromium.org/2133793003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
PTAL https://codereview.chromium.org/2133793003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/settings_shared_css.html (right): https://codereview.chromium.org/2133793003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/settings_shared_css.html:63: iron-dropdown .dropdown-content > * { On 2016/07/09 00:12:05, Dan Beam wrote: > i think paper-item is better than "> *" here because: > a) it works even if the <paper-item> is wrapped with another tag, i.e. > <div><paper-item> > b) separators seem fairly likely in the future (and they probably shouldn't be > actionable) I thought the plan was to move away from paper-item? I guess we can cross that bridge when we get there. Changed.
On 2016/07/11 17:01:58, stevenjb wrote: > https://cs.chromium.org/search/?q=on-tap+file:%5Esrc/chrome/browser/resources... > > First two instances don't have 'actionable'. Maybe those are outliers? They don't have an [actionable] attribute, but they have this: .list-button { @apply(--settings-actionable); } [... snip ...] <div class="list-item list-button" id="addPage" on-tap="onAddPageTap_"> $i18n{onStartupAddNewPage} </div> > > > On Fri, Jul 8, 2016 at 5:09 PM, <mailto:dbeam@chromium.org> wrote: > > > > > > > > https://codereview.chromium.org/2133793003/diff/20001/chrome/browser/resource... > > File > > > > > chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.html > > (left): > > > > > > > https://codereview.chromium.org/2133793003/diff/20001/chrome/browser/resource... > > > > > chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.html:40: > > visible. --> > > On 2016/07/08 23:50:05, stevenjb wrote: > > > On 2016/07/08 23:08:08, Dan Beam wrote: > > > > why are you removing this? > > > > > > I looked through the code and found a number of divs with on-tap and > > no > > > 'actionable' property and no comment. It wasn't clear to me what was > > special > > > about this one, other than the TODO below which I believe is now > > resolved. > > > > which ones? > > > > > > > > I can restore it if you think it's helpful, but maybe we can clarify > > it? > > > > how would you clarify it? > > > > https://codereview.chromium.org/2133793003/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
new code lgtm
On 2016/07/11 21:04:51, Dan Beam wrote: > On 2016/07/11 17:01:58, stevenjb wrote: > > > https://cs.chromium.org/search/?q=on-tap+file:%5Esrc/chrome/browser/resources... > > > > First two instances don't have 'actionable'. Maybe those are outliers? > > They don't have an [actionable] attribute, but they have this: > > .list-button { > @apply(--settings-actionable); > } > > [... snip ...] > > <div class="list-item list-button" id="addPage" on-tap="onAddPageTap_"> > $i18n{onStartupAddNewPage} > </div> > > > > > > > On Fri, Jul 8, 2016 at 5:09 PM, <mailto:dbeam@chromium.org> wrote: > > > > > > > > > > > > > > https://codereview.chromium.org/2133793003/diff/20001/chrome/browser/resource... > > > File > > > > > > > > > chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.html > > > (left): > > > > > > > > > > > > https://codereview.chromium.org/2133793003/diff/20001/chrome/browser/resource... > > > > > > > > > chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.html:40: > > > visible. --> > > > On 2016/07/08 23:50:05, stevenjb wrote: > > > > On 2016/07/08 23:08:08, Dan Beam wrote: > > > > > why are you removing this? > > > > > > > > I looked through the code and found a number of divs with on-tap and > > > no > > > > 'actionable' property and no comment. It wasn't clear to me what was > > > special > > > > about this one, other than the TODO below which I believe is now > > > resolved. > > > > > > which ones? > > > > > > > > > > > I can restore it if you think it's helpful, but maybe we can clarify > > > it? > > > > > > how would you clarify it? > > > > > > https://codereview.chromium.org/2133793003/ > > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. Ah, I see. I should have thought to check the class list more carefully. I'd already restored the comment (and clarified it for my own future reference, for better or for worse).
The CQ bit was checked by stevenjb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/2133793003/#ps60001 (title: "More feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Unify iron-dropdown CSS Moves shared CSS for iron-dropdown to settings_shared_css.html. Also makes dropdown items actionable. BUG=621731 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Unify iron-dropdown CSS Moves shared CSS for iron-dropdown to settings_shared_css.html. Also makes dropdown items actionable. BUG=621731 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/53b690eb87b6c8786574622243893049d3745bde Cr-Commit-Position: refs/heads/master@{#404756} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/53b690eb87b6c8786574622243893049d3745bde Cr-Commit-Position: refs/heads/master@{#404756} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
