----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58874/#review174590 -----------------------------------------------------------
Fix it, then Ship it! Ok, I understand now what's going on in this change. I gave some suggestions for comments / naming to clarify this. It feels like a hack however, since I would expect the breadcrumb '/' characters to be getting copied. Do you understand why they're not getting copied? If not, please leave a TODO to look into that. src/webui/master/static/browse.html Lines 15-16 (original), 15-16 (patched) <https://reviews.apache.org/r/58874/#comment247781> This comment seems misleading, since we're not doing any stripping here, are we? It seems that like Tomasz said, the spaces are being removed because now there are no spaces or newlines between the closing `</a>` and `</li>` tags? How about this? ``` <!-- We want to ensure that if the user highlights the path breadcrumb, and copies it, they will receive a /path/without/spaces that they can then paste into a terminal, or elsewhere. In order to do this, we have to ensure there is no whitespace within the <a> tag contents. Also, we have to inject a hidden '/' character because the slashes in the breadcrumb are not copied. TODO(haosdent): Figure out why the breadcrumb's '/' characters are not being copied. --> ``` Is this accurate? src/webui/master/static/browse.html Lines 17-20 (original), 17-20 (patched) <https://reviews.apache.org/r/58874/#comment247782> I suspect, if my understanding is correct, you could do the following format: ``` <li ng-repeat="dir in path.split('/')"> <a href="#/agents/{{agent_id}}/browse?path={{ encodeURIComponent(path.split('/').slice(0, $index + 1).join('/')) }}">{{dir}}</a> <span class="copy-placeholder">/</span> </li> ``` Does that also insert a space? src/webui/master/static/css/mesos.css Lines 179-181 (patched) <https://reviews.apache.org/r/58874/#comment247778> Ok I think I understand now, in that this is just making the text invisible using a 0 size font..? If so, how about calling this `hidden-text`? ``` // The `hidden-text` class hides text by making the font size 0. // This can be used, for example, to insert text that will be // added when the user highlights and copies, while leaving // the text hidden from the user. ``` This still feels like a hack to me, have you tried looking into whether we can just update the `breadcrumb` class to have the slashes show up when copying? - Benjamin Mahler On May 8, 2017, 8:01 a.m., haosdent huang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58874/ > ----------------------------------------------------------- > > (Updated May 8, 2017, 8:01 a.m.) > > > Review request for mesos, Benjamin Mahler and Tomasz Janiszewski. > > > Bugs: MESOS-7468 > https://issues.apache.org/jira/browse/MESOS-7468 > > > Repository: mesos > > > Description > ------- > > Stripped spaces between directory elements in WebUI. > > > Diffs > ----- > > src/webui/master/static/browse.html > b9849197227b06df348789a49348e2b5d4cfd2ae > src/webui/master/static/css/mesos.css > 9f3de5427071fc61d3791c4bc2a660368c2cd3c2 > > > Diff: https://reviews.apache.org/r/58874/diff/3/ > > > Testing > ------- > > > File Attachments > ---------------- > > strip_space.gif > > https://reviews.apache.org/media/uploaded/files/2017/05/03/79afd0b1-5fb1-437c-a91c-732009af8fe3__strip_space.gif > > > Thanks, > > haosdent huang > >
