mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 58874: Stripped spaces between directory elements in WebUI.
Date Thu, 11 May 2017 00:38:52 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message