Fix tooltip and move replace in selected cells button. #567

jdfreder · 2015-10-09T14:58:03Z

partially closes #558

jasongrout · 2015-10-09T16:33:35Z

notebook/static/notebook/js/searchandreplace.js

-              .append(onlySelectedButton)
-          )
+          $('<div/>')
+              .addClass('input-group')


why not just append the repl, rather than appending the input group div, and then putting repl inside that?

That causes it to render incorrectly.

see my PR to yours, which does what I was suggesting (though I may have not been clear) and seems to render fine, at least on Chrome where I was testing.

ellisonbg · 2015-10-12T22:05:01Z

This is a good start to that. A few points:

After playing with it for a while, I find a couple of borders are missing. Here is a screenshot:

The left border of the "Replace" text area and the right border of the selection checkbox.

jasongrout · 2015-10-12T22:10:29Z

@jdfreder - I put in a PR to your PR.

@ellisonbg - I also saw that and I fixed that in my PR to @jdfreder.

jasongrout · 2015-10-12T22:11:11Z

@jdfreder, I'm also seeing a 1-2 pixel difference between the buttons and the input area. The buttons I think are sized to 32px, but the input area isn't, so there's a dissonance between the two.

ellisonbg · 2015-10-12T22:11:55Z

notebook/static/notebook/js/searchandreplace.js

@@ -166,14 +165,14 @@ define(function(require){
        .addClass("fa fa-check-square-o")
      )
      .attr('data-toggle','button')


Let's also replace the faux checkbox button with a real checkbox!

or change the icon to fa-square-o (to indicated a "selected" border)

Because the button is already a toggle button, I don't think it makes sense to add a checkbox too. I'd either need to sync the check state with the button state or keep the checkbox static. Not to mention, on some operating systems/browsers, check boxes aren't that pretty.

The fa-square-o idea is easy though, @ellisonbg would you like that?

Search and replace

jdfreder · 2015-10-12T22:12:53Z

@jasongrout ah, that missing border is what I thought was a consequence of simplifying the border. I didn't notice it was gone even with the code as is. I merged your PR

Carreau · 2015-10-14T18:39:53Z

-1 to move the checkbox. The re and case sensitive change the meaning of the text which is in the search field. The check box apply only to where it will apply. After using the SnR for some time, especially with the keyboard, I find out that I was selecting the "replace only in selected" only after entering the replace text. and having after the replace filed made it much more convenient to reach with tab.

I was at the beginning with the other 2 button, and it was annoying. I moved it after on purpose.

ellisonbg · 2015-10-14T18:51:11Z

Shouldn't the checkbox also affect which cells the search is done in. If
you want to do a replace, why would you search more widely that you know
you will replace on. Also, I think it would be very useful to be able to
"just search" on selected cells.

On Wed, Oct 14, 2015 at 2:39 PM, Matthias Bussonnier <
[email protected]> wrote:

-1 to move the checkbox. The re and case sensitive change the meaning of
the text which is in the search field. The check box apply only to where it
will apply. After using the SnR for some time, especially with the
keyboard, I find out that I was selecting the "replace only in selected"
only after entering the replace text. and having after the replace
filed made it much more convenient to reach with tab.

I was at the beginning with the other 2 button, and it was annoying. I
moved it after on purpose.

—
Reply to this email directly or view it on GitHub
#567 (comment).

Brian E. Granger
Associate Professor of Physics and Data Science
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
[email protected] and [email protected]

Carreau · 2015-10-14T19:03:22Z

Yes for "Just search", though harder to do that SnR, not sure about the UI for that.

I think the browser Search is better for just search.

From my personal use, I know I want to search and replace, it just append to replace in places where I am not expecting to find what I am searching so my workflow is the following:

search for Foo
replace by Bar
oh it also replace in place I am not expecting.
deselect where I do not want it to replace.

Technically I would like a checkbox next to each preview, and be able to individually deselect some results (à la textmate).

Hence why I placed the checkbox after as it is the last action I before validating.

ellisonbg · 2015-10-14T19:16:31Z

I should clarify my feelings about the select button:

First, let's make it a real checkbox rather than a
faux-checkbox-button--pretending-to-be-a-checkbox.
Second, make sure it has a tolltip (probably does)
Possibly move to the beginning of the replace so it has symmetry with the
other buttons being on the left.
With these changes and your description, I think it would be fine to
leave below with replace.

On Wed, Oct 14, 2015 at 3:03 PM, Matthias Bussonnier <
[email protected]> wrote:

Yes for "Just search", though harder to do that SnR, not sure about the UI
for that.

I think the browser Search is better for just search.

From my personal use, I know I want to search and replace, it just append
to replace in places where I am not expecting to find what I am searching
so my workflow is the following:

search for Foo

replace by Bar

oh it also replace in place I am not expecting.

deselect where I do not want it to replace.

Technically I would like a checkbox next to each preview, and be able to
individually deselect some results (à la textmate).

Hence why I placed the checkbox after as it is the last action I before
validating.

—
Reply to this email directly or view it on GitHub
#567 (comment).

Brian E. Granger
Associate Professor of Physics and Data Science
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
[email protected] and [email protected]

Carreau · 2015-10-14T20:42:39Z

First, let's make it a real checkbox rather than a
faux-checkbox-button--pretending-to-be-a-checkbox.

No objection. Design-wise, it might be less pretty, but fair enough.

Possibly move to the beginning of the replace so it has symmetry with the
other buttons being on the left.

it make keyboad usage a bit harder. I would prefer tab/shift tab to move quickly between search and replace field, if we put it before, it will be 2 press. We can change the tabindex, but then the tab selection order might be confusing.

No objection to try, though I want to keep the keyboard usability in mind.

jdfreder · 2015-10-14T22:15:40Z

First, let's make it a real checkbox rather than a
faux-checkbox-button--pretending-to-be-a-checkbox.

I'm still slightly minus one on this for the reasons I stated above, which you may have missed:

Because the button is already a toggle button, I don't think it makes sense to add a checkbox too. I'd either need to sync the check state with the button state or keep the checkbox static. Not to mention, on some operating systems/browsers, check boxes aren't that pretty.

Carreau · 2015-10-21T22:47:10Z

Merging to test it.

Fix tooltip and move replace in selected cells button.

Fix tooltip and move replace in selected cells button.

8526ff7

jdfreder added this to the 4.1 milestone Oct 9, 2015

jdfreder mentioned this pull request Oct 9, 2015

Search and Replace issues #558

Closed

7 tasks

jasongrout reviewed Oct 9, 2015
View reviewed changes

restore left border on last input

85ebbd3

simplify find/replace form

de15702

ellisonbg reviewed Oct 12, 2015
View reviewed changes

Merge pull request #3 from jasongrout/findreplace

a729745

Search and replace

Carreau added a commit that referenced this pull request Oct 21, 2015

Merge pull request #567 from jdfreder/sandr

c40400c

Fix tooltip and move replace in selected cells button.

Carreau merged commit c40400c into jupyter:master Oct 21, 2015

github-actions bot added the status:resolved-locked label Apr 25, 2021

github-actions bot locked as resolved and limited conversation to collaborators Apr 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix tooltip and move replace in selected cells button. #567

Fix tooltip and move replace in selected cells button. #567

jdfreder commented Oct 9, 2015

jasongrout Oct 9, 2015

jdfreder Oct 9, 2015

jasongrout Oct 12, 2015

ellisonbg commented Oct 12, 2015

jasongrout commented Oct 12, 2015

jasongrout commented Oct 12, 2015

ellisonbg Oct 12, 2015

jasongrout Oct 13, 2015

jdfreder Oct 13, 2015

jdfreder commented Oct 12, 2015

Carreau commented Oct 14, 2015

ellisonbg commented Oct 14, 2015

Carreau commented Oct 14, 2015

ellisonbg commented Oct 14, 2015

Carreau commented Oct 14, 2015

jdfreder commented Oct 14, 2015

Carreau commented Oct 21, 2015

Fix tooltip and move replace in selected cells button. #567

Fix tooltip and move replace in selected cells button. #567

Conversation

jdfreder commented Oct 9, 2015

jasongrout Oct 9, 2015

Choose a reason for hiding this comment

jdfreder Oct 9, 2015

Choose a reason for hiding this comment

jasongrout Oct 12, 2015

Choose a reason for hiding this comment

ellisonbg commented Oct 12, 2015

jasongrout commented Oct 12, 2015

jasongrout commented Oct 12, 2015

ellisonbg Oct 12, 2015

Choose a reason for hiding this comment

jasongrout Oct 13, 2015

Choose a reason for hiding this comment

jdfreder Oct 13, 2015

Choose a reason for hiding this comment

jdfreder commented Oct 12, 2015

Carreau commented Oct 14, 2015

ellisonbg commented Oct 14, 2015

Carreau commented Oct 14, 2015

ellisonbg commented Oct 14, 2015

Carreau commented Oct 14, 2015

jdfreder commented Oct 14, 2015

Carreau commented Oct 21, 2015