Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2530763002:
[ash-md] Adjusts layout of lists with sticky header rows to match specs (Closed)
Description[ash-md] Adjusts layout of lists with sticky header rows to match specs
This CL adds a separator above the sticky header rows when those rows
are not at the top of the list. The visible separator line is drawn by
this separator and is no longer a part of the sub-section header row.
The sticky row decoration is adjusted such that a separator is painted
only when one header is pushing another since in all other cases the
explicitly added separator is painting the line.
Since VPN page is not using sticky headers yet some special treatment
is added there to adjust the height of the header row when it is the
first item in the list.
BUG=668545
TEST=Manual visual inspection
Committed: https://crrev.com/d66d5e2cd8ff8ae53f607dd85534d130b33048f4
Cr-Commit-Position: refs/heads/master@{#434832}
Patch Set 1 #Patch Set 2 : [ash-md] Adjusts layout of lists with sticky header rows to match specs (horizontal offsets) #
Total comments: 2
Patch Set 3 : [ash-md] Adjusts layout of lists with sticky header rows to match specs (rebased) #
Total comments: 35
Patch Set 4 : [ash-md] Adjusts layout of lists with sticky header rows to match specs (some comments and rebased) #Patch Set 5 : [ash-md] Adjusts layout of lists with sticky header rows to match specs (factory) #
Total comments: 2
Patch Set 6 : [ash-md] Adjusts layout of lists with sticky header rows to match specs (restored header row as the… #Patch Set 7 : [ash-md] Adjusts layout of lists with sticky header rows to match specs (simpler) #Patch Set 8 : [ash-md] Adjusts layout of lists with sticky header rows to match specs (use TriView in Network pag… #
Total comments: 2
Patch Set 9 : [ash-md] Adjusts layout of lists with sticky header rows to match specs (cleanup of shill) #
Messages
Total messages: 44 (31 generated)
varkha@chromium.org changed reviewers: + tdanderson@chromium.org
tdanderson@, can you please take a look? Thanks! https://codereview.chromium.org/2530763002/diff/20001/chromeos/dbus/fake_shil... File chromeos/dbus/fake_shill_manager_client.cc (right): https://codereview.chromium.org/2530763002/diff/20001/chromeos/dbus/fake_shil... chromeos/dbus/fake_shill_manager_client.cc:41: int s_extra_wifi_networks = 20; The changes in this file are for debugging and will be reverted before landing.
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [ash-md] Adjusts layout of lists with sticky header rows to match specs This CL adds a separator above the sticky header rows when those rows are not at the top of the list. The visible separator line is drawn by this separator and is no longer a part of the sub-section header row. The sticky row decoration is adjusted such that a separator is painted only when one header is pushing another since in all other cases the explicitly added separator is painting the line. Since VPN page is not using sticky headers yet some special treatment is added there to adjust the height of the header row when it is the first item in the list. BUG=663451 TEST=Manual visual inspection ========== to ========== [ash-md] Adjusts layout of lists with sticky header rows to match specs This CL adds a separator above the sticky header rows when those rows are not at the top of the list. The visible separator line is drawn by this separator and is no longer a part of the sub-section header row. The sticky row decoration is adjusted such that a separator is painted only when one header is pushing another since in all other cases the explicitly added separator is painting the line. Since VPN page is not using sticky headers yet some special treatment is added there to adjust the height of the header row when it is the first item in the list. BUG=668545 TEST=Manual visual inspection ==========
Please see my comments below. https://codereview.chromium.org/2530763002/diff/20001/chromeos/dbus/fake_shil... File chromeos/dbus/fake_shill_manager_client.cc (right): https://codereview.chromium.org/2530763002/diff/20001/chromeos/dbus/fake_shil... chromeos/dbus/fake_shill_manager_client.cc:41: int s_extra_wifi_networks = 20; On 2016/11/24 06:46:54, varkha wrote: > The changes in this file are for debugging and will be reverted before landing. Acknowledged. https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/chrom... File ash/common/system/chromeos/network/network_list_md.cc (right): https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/network/network_list_md.cc:450: // Keep an index of the last inserted child. Similar to my previous comment, I think this description is wrong. Doesn't |index| actually represent the location where the next child should be inserted? https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/network/network_list_md.cc:604: *separator_view = nullptr; Instead of deleting, would it be sufficient to just call SetVisible(false)? (Similarly, call SetVisible(true) in the if-case if the separator has already been created). When the separator is created it's added as a child view of |this|, so |this| takes responsibility of the separator's lifetime management I believe. https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/network/network_list_md.cc:609: PlaceViewAtIndex(*separator_view, child_index++); Can you move this to directly after line 600, i.e., 599 if (!*separator_view) 600 *separator_view = ... 601 PlaceViewAtIndex(*separator_view, child_index++); then delete line 608? https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/chrom... File ash/common/system/chromeos/network/network_list_md.h (right): https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/network/network_list_md.h:5: #ifndef ASH_COMMON_SYSTEM_CHROMEOS_NETWORK_NETWORK_LIST_MD_H_ nit: please change the BUG= line in your CL to BUG=668545 (the bug you currently have listed already has Merge-Request set and we should only be requesting one merge per bug). https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/network/network_list_md.h:95: // |child_index|. Returns the last occupied child index. I think the comment you've added should instead read "Returns the index where the next child should be inserted, i.e., the index directly after the last inserted child." - that seems to be what you're doing in the implementation. https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/chrom... File ash/common/system/chromeos/network/vpn_list_view.cc (right): https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/network/vpn_list_view.cc:125: auto box_layout = base::MakeUnique<views::BoxLayout>( Please add a TODO to consider making a factory function for this type of row instead of modifying the "CreateDefaultRowView()" at creation site. Though it may be worth double-checking with Ben; if he has any changes planned in 56 for how CreateDefaultRowView() behaves then it may inadvertently impact the layout here. https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/network/vpn_list_view.cc:146: // contents, so only add that padding when the list was not |empty|. So just to be sure, you are not doing any of this special handling to suppress the extra 4px when the header is the first thing in network_list_md.cc *because* the network headers are sticky and thus are able to handle that already? https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/network/vpn_list_view.cc:152: GetTrayConstant(TRAY_POPUP_ITEM_LEFT_INSET), So you're subtracting TRAY_POPUP_ITEM_LEFT_INSET from the left margin to compensate for what TriView is doing? https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/network/vpn_list_view.cc:285: void VPNListNetworkEntry::UpdateFromNetworkState( Please revert your changes to UpdateFromNetworkState(). Yi has a CL in progress that will be addressing the internal layout of rows which contain an MdTextButton, which includes changes to this function (https://chromiumcodereview.appspot.com/2512883002/) - I think they more properly belong within the scope of her CL, and removing them here makes your CL smaller. https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/tray/... File ash/common/system/tray/tray_details_view.cc (right): https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/tray/... ash/common/system/tray/tray_details_view.cc:162: bool separator; I suggest renaming to |draw_separator_below| or similar for readability at call sites. Alternatively, consider a name that represents what the variable is actually tracking, such as |joined_to_previous_header|. https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/tray/... ash/common/system/tray/tray_details_view.cc:173: header.separator = false; Consider moving this line directly above line 172 (outside the if) and then deleting line 185. https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/tray/... ash/common/system/tray/tray_details_view.cc:174: header_view->SetY(header.natural_offset); Is there a reason for moving this line before previous_header = &header? https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/tray/... ash/common/system/tray/tray_details_view.cc:179: previous_header->view->y() <= scroll_offset + header_view->height()) { Double checking my understanding: you changed this to <= so that a separator is drawn at the time when the previous header and this header first come into contact, but the current header hasn't started to be displaced from its sticky position? https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/tray/... File ash/common/system/tray/tray_popup_utils.h (right): https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/tray/... ash/common/system/tray/tray_popup_utils.h:172: // in a detailed view above the header rows. Caller assumes ownership of the nit: "above the sub-header rows". I would also prefer the function name to change to CreateListSubHeaderSeparator(). This makes the naming consistent with TrayPopupItemStyle::FontStyle::SUB_HEADER that you recently added. https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/tray/... File ash/common/system/tray/tri_view.cc (right): https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/tray/... ash/common/system/tray/tri_view.cc:131: } Thanks!
https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/chrom... File ash/common/system/chromeos/network/vpn_list_view.cc (right): https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/network/vpn_list_view.cc:125: auto box_layout = base::MakeUnique<views::BoxLayout>( On 2016/11/24 23:37:53, tdanderson wrote: > Please add a TODO to consider making a factory function for this type of row > instead of modifying the "CreateDefaultRowView()" at creation site. Though it > may be worth double-checking with Ben; if he has any changes planned in 56 for > how CreateDefaultRowView() behaves then it may inadvertently impact the layout > here. Please also make sure Ben's change at https://chromiumcodereview.appspot.com/2531713002/ (which will land before this one) does not mess up the layout in this CL.
bruthig@chromium.org changed reviewers: + bruthig@chromium.org
Drive by... https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/chrom... File ash/common/system/chromeos/network/vpn_list_view.cc (right): https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/network/vpn_list_view.cc:125: auto box_layout = base::MakeUnique<views::BoxLayout>( On 2016/11/24 23:37:53, tdanderson wrote: > Please add a TODO to consider making a factory function for this type of row > instead of modifying the "CreateDefaultRowView()" at creation site. Though it > may be worth double-checking with Ben; if he has any changes planned in 56 for > how CreateDefaultRowView() behaves then it may inadvertently impact the layout > here. +1 to adding a factory function like TPU::CreateSubHeaderRow() that returns a correctly configured TriView.
https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/chrom... File ash/common/system/chromeos/network/network_list_md.cc (right): https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/network/network_list_md.cc:450: // Keep an index of the last inserted child. On 2016/11/24 23:37:53, tdanderson wrote: > Similar to my previous comment, I think this description is wrong. Doesn't > |index| actually represent the location where the next child should be inserted? Done. https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/network/network_list_md.cc:604: *separator_view = nullptr; On 2016/11/24 23:37:53, tdanderson wrote: > Instead of deleting, would it be sufficient to just call SetVisible(false)? > (Similarly, call SetVisible(true) in the if-case if the separator has already > been created). When the separator is created it's added as a child view of > |this|, so |this| takes responsibility of the separator's lifetime management I > believe. Done. Simpler indeed. https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/network/network_list_md.cc:609: PlaceViewAtIndex(*separator_view, child_index++); On 2016/11/24 23:37:53, tdanderson wrote: > Can you move this to directly after line 600, i.e., > > 599 if (!*separator_view) > 600 *separator_view = ... > 601 PlaceViewAtIndex(*separator_view, child_index++); > > then delete line 608? Done. https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/chrom... File ash/common/system/chromeos/network/network_list_md.h (right): https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/network/network_list_md.h:5: #ifndef ASH_COMMON_SYSTEM_CHROMEOS_NETWORK_NETWORK_LIST_MD_H_ On 2016/11/24 23:37:53, tdanderson wrote: > nit: please change the BUG= line in your CL to BUG=668545 (the bug you currently > have listed already has Merge-Request set and we should only be requesting one > merge per bug). Done. https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/network/network_list_md.h:95: // |child_index|. Returns the last occupied child index. On 2016/11/24 23:37:53, tdanderson wrote: > I think the comment you've added should instead read "Returns the index where > the next child should be inserted, i.e., the index directly after the last > inserted child." - that seems to be what you're doing in the implementation. Done. https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/chrom... File ash/common/system/chromeos/network/vpn_list_view.cc (right): https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/network/vpn_list_view.cc:125: auto box_layout = base::MakeUnique<views::BoxLayout>( On 2016/11/25 00:08:54, bruthig wrote: > On 2016/11/24 23:37:53, tdanderson wrote: > > Please add a TODO to consider making a factory function for this type of row > > instead of modifying the "CreateDefaultRowView()" at creation site. Though it > > may be worth double-checking with Ben; if he has any changes planned in 56 for > > how CreateDefaultRowView() behaves then it may inadvertently impact the layout > > here. > > +1 to adding a factory function like TPU::CreateSubHeaderRow() that returns a > correctly configured TriView. Acknowledged. https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/network/vpn_list_view.cc:125: auto box_layout = base::MakeUnique<views::BoxLayout>( On 2016/11/24 23:59:52, tdanderson wrote: > On 2016/11/24 23:37:53, tdanderson wrote: > > Please add a TODO to consider making a factory function for this type of row > > instead of modifying the "CreateDefaultRowView()" at creation site. Though it > > may be worth double-checking with Ben; if he has any changes planned in 56 for > > how CreateDefaultRowView() behaves then it may inadvertently impact the layout > > here. > > Please also make sure Ben's change at > https://chromiumcodereview.appspot.com/2531713002/ (which will land before this > one) does not mess up the layout in this CL. Acknowledged. https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/network/vpn_list_view.cc:125: auto box_layout = base::MakeUnique<views::BoxLayout>( On 2016/11/24 23:37:53, tdanderson wrote: > Please add a TODO to consider making a factory function for this type of row > instead of modifying the "CreateDefaultRowView()" at creation site. Though it > may be worth double-checking with Ben; if he has any changes planned in 56 for > how CreateDefaultRowView() behaves then it may inadvertently impact the layout > here. Acknowledged. https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/network/vpn_list_view.cc:146: // contents, so only add that padding when the list was not |empty|. On 2016/11/24 23:37:53, tdanderson wrote: > So just to be sure, you are not doing any of this special handling to suppress > the extra 4px when the header is the first thing in network_list_md.cc *because* > the network headers are sticky and thus are able to handle that already? Yes, if configured as sticky this would not be necessary. See https://cs.chromium.org/chromium/src/ash/common/system/tray/tray_details_view... https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/network/vpn_list_view.cc:152: GetTrayConstant(TRAY_POPUP_ITEM_LEFT_INSET), On 2016/11/24 23:37:53, tdanderson wrote: > So you're subtracting TRAY_POPUP_ITEM_LEFT_INSET from the left margin to > compensate for what TriView is doing? Yes. Let me see if adding a factory would isolate this code there. https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/network/vpn_list_view.cc:285: void VPNListNetworkEntry::UpdateFromNetworkState( On 2016/11/24 23:37:53, tdanderson wrote: > Please revert your changes to UpdateFromNetworkState(). Yi has a CL in progress > that will be addressing the internal layout of rows which contain an > MdTextButton, which includes changes to this function > (https://chromiumcodereview.appspot.com/2512883002/) - I think they more > properly belong within the scope of her CL, and removing them here makes your CL > smaller. Done. https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/tray/... File ash/common/system/tray/tray_details_view.cc (right): https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/tray/... ash/common/system/tray/tray_details_view.cc:162: bool separator; On 2016/11/24 23:37:54, tdanderson wrote: > I suggest renaming to |draw_separator_below| or similar for readability at call > sites. Alternatively, consider a name that represents what the variable is > actually tracking, such as |joined_to_previous_header|. Done. https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/tray/... ash/common/system/tray/tray_details_view.cc:173: header.separator = false; On 2016/11/24 23:37:54, tdanderson wrote: > Consider moving this line directly above line 172 (outside the if) and then > deleting line 185. Done. https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/tray/... ash/common/system/tray/tray_details_view.cc:174: header_view->SetY(header.natural_offset); On 2016/11/24 23:37:54, tdanderson wrote: > Is there a reason for moving this line before previous_header = &header? Just seemed nicer with the now moved assignment. Moved back. https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/tray/... ash/common/system/tray/tray_details_view.cc:179: previous_header->view->y() <= scroll_offset + header_view->height()) { On 2016/11/24 23:37:54, tdanderson wrote: > Double checking my understanding: you changed this to <= so that a separator is > drawn at the time when the previous header and this header first come into > contact, but the current header hasn't started to be displaced from its sticky > position? Yes, exactly. Otherwise I would get a flash of no separator when the rows are exactly touching. https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/tray/... File ash/common/system/tray/tray_popup_utils.h (right): https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/tray/... ash/common/system/tray/tray_popup_utils.h:172: // in a detailed view above the header rows. Caller assumes ownership of the On 2016/11/24 23:37:54, tdanderson wrote: > nit: "above the sub-header rows". I would also prefer the function name to > change to CreateListSubHeaderSeparator(). This makes the naming consistent with > TrayPopupItemStyle::FontStyle::SUB_HEADER that you recently added. Done. https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/tray/... File ash/common/system/tray/tri_view.cc (right): https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/tray/... ash/common/system/tray/tri_view.cc:131: } On 2016/11/24 23:37:54, tdanderson wrote: > Thanks! Acknowledged.
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Patchset #4 (id:60001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/chrom... File ash/common/system/chromeos/network/vpn_list_view.cc (right): https://codereview.chromium.org/2530763002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/network/vpn_list_view.cc:125: auto box_layout = base::MakeUnique<views::BoxLayout>( On 2016/11/25 00:08:54, bruthig wrote: > On 2016/11/24 23:37:53, tdanderson wrote: > > Please add a TODO to consider making a factory function for this type of row > > instead of modifying the "CreateDefaultRowView()" at creation site. Though it > > may be worth double-checking with Ben; if he has any changes planned in 56 for > > how CreateDefaultRowView() behaves then it may inadvertently impact the layout > > here. > > +1 to adding a factory function like TPU::CreateSubHeaderRow() that returns a > correctly configured TriView. Done. See if this is what you had in mind.
https://codereview.chromium.org/2530763002/diff/100001/ash/common/system/chro... File ash/common/system/chromeos/network/vpn_list_view.cc (right): https://codereview.chromium.org/2530763002/diff/100001/ash/common/system/chro... ash/common/system/chromeos/network/vpn_list_view.cc:141: tri_view_->SetBorder(views::CreateEmptyBorder( How much of this is going to live and how much is going to die when addressing the TODO? Anything that is going to live should be pushed into TPIS::CreateSubHeaderRowView(). You could even move it all there now and have it take in the |empty| param. I'm assuming this layout should be the same for Networking sub-section headers as well if/when it is fixed to use a TriView? I've been trying to keep any TriView border configurations inside of TPIS and if anything custom is required by TPIS clients they can update the borders of the individual containers via TriView::SetContainerBorder().
https://codereview.chromium.org/2530763002/diff/100001/ash/common/system/chro... File ash/common/system/chromeos/network/vpn_list_view.cc (right): https://codereview.chromium.org/2530763002/diff/100001/ash/common/system/chro... ash/common/system/chromeos/network/vpn_list_view.cc:141: tri_view_->SetBorder(views::CreateEmptyBorder( On 2016/11/28 21:43:12, bruthig wrote: > How much of this is going to live and how much is going to die when addressing > the TODO? Anything that is going to live should be pushed into > TPIS::CreateSubHeaderRowView(). You could even move it all there now and have > it take in the |empty| param. I'm assuming this layout should be the same for > Networking sub-section headers as well if/when it is fixed to use a TriView? > > I've been trying to keep any TriView border configurations inside of TPIS and if > anything custom is required by TPIS clients they can update the borders of the > individual containers via TriView::SetContainerBorder(). It is manipulated depending on whether it is the first row (not just at creation time). That will not be needed when the header is setup as a sticky header row. I can still add a parameter to ctor - that will make things a bit less verbose I think.
Patchset #7 (id:140001) has been deleted
Patchset #7 (id:160001) has been deleted
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM https://codereview.chromium.org/2530763002/diff/200001/chromeos/dbus/fake_shi... File chromeos/dbus/fake_shill_manager_client.cc (right): https://codereview.chromium.org/2530763002/diff/200001/chromeos/dbus/fake_shi... chromeos/dbus/fake_shill_manager_client.cc:921: services->AddService("/service/vpn3", "vpn3_guid", "vpn3" /* name */, Don't forget to delete all of this before committing.
https://codereview.chromium.org/2530763002/diff/200001/chromeos/dbus/fake_shi... File chromeos/dbus/fake_shill_manager_client.cc (right): https://codereview.chromium.org/2530763002/diff/200001/chromeos/dbus/fake_shi... chromeos/dbus/fake_shill_manager_client.cc:921: services->AddService("/service/vpn3", "vpn3_guid", "vpn3" /* name */, On 2016/11/28 23:55:26, tdanderson wrote: > Don't forget to delete all of this before committing. Of course. Done.
The CQ bit was checked by varkha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdanderson@chromium.org Link to the patchset: https://codereview.chromium.org/2530763002/#ps220001 (title: "[ash-md] Adjusts layout of lists with sticky header rows to match specs (cleanup of shill)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1480378550327560,
"parent_rev": "1420c138f158123e376a52d9e2f793be6316b95a", "commit_rev":
"2aad13f86d517d82b32bad56d50353517933e23d"}
Message was sent while issue was closed.
Description was changed from ========== [ash-md] Adjusts layout of lists with sticky header rows to match specs This CL adds a separator above the sticky header rows when those rows are not at the top of the list. The visible separator line is drawn by this separator and is no longer a part of the sub-section header row. The sticky row decoration is adjusted such that a separator is painted only when one header is pushing another since in all other cases the explicitly added separator is painting the line. Since VPN page is not using sticky headers yet some special treatment is added there to adjust the height of the header row when it is the first item in the list. BUG=668545 TEST=Manual visual inspection ========== to ========== [ash-md] Adjusts layout of lists with sticky header rows to match specs This CL adds a separator above the sticky header rows when those rows are not at the top of the list. The visible separator line is drawn by this separator and is no longer a part of the sub-section header row. The sticky row decoration is adjusted such that a separator is painted only when one header is pushing another since in all other cases the explicitly added separator is painting the line. Since VPN page is not using sticky headers yet some special treatment is added there to adjust the height of the header row when it is the first item in the list. BUG=668545 TEST=Manual visual inspection ==========
Message was sent while issue was closed.
Committed patchset #9 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== [ash-md] Adjusts layout of lists with sticky header rows to match specs This CL adds a separator above the sticky header rows when those rows are not at the top of the list. The visible separator line is drawn by this separator and is no longer a part of the sub-section header row. The sticky row decoration is adjusted such that a separator is painted only when one header is pushing another since in all other cases the explicitly added separator is painting the line. Since VPN page is not using sticky headers yet some special treatment is added there to adjust the height of the header row when it is the first item in the list. BUG=668545 TEST=Manual visual inspection ========== to ========== [ash-md] Adjusts layout of lists with sticky header rows to match specs This CL adds a separator above the sticky header rows when those rows are not at the top of the list. The visible separator line is drawn by this separator and is no longer a part of the sub-section header row. The sticky row decoration is adjusted such that a separator is painted only when one header is pushing another since in all other cases the explicitly added separator is painting the line. Since VPN page is not using sticky headers yet some special treatment is added there to adjust the height of the header row when it is the first item in the list. BUG=668545 TEST=Manual visual inspection Committed: https://crrev.com/d66d5e2cd8ff8ae53f607dd85534d130b33048f4 Cr-Commit-Position: refs/heads/master@{#434832} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/d66d5e2cd8ff8ae53f607dd85534d130b33048f4 Cr-Commit-Position: refs/heads/master@{#434832} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
