Fix for issue 4602. #4661

zaggino · 2013-08-06T00:16:06Z

My go on a #4602
Please review.

zaggino · 2013-08-06T00:50:38Z

Can this be also solved by using https://developer.mozilla.org/en-US/docs/Web/CSS/calc ?

TomMalbran · 2013-08-06T03:58:44Z

Might want to wait until #4303 gets merged, which hopefully will be soon, since I changed the template a bit and it will now display arrows to show more results which shouldn't be hidden.

Instead of calc, you can achieve similar results with box-sizing and padding.

zaggino · 2013-08-06T04:13:02Z

@TomMalbran thx, I'll give it a go with "box-sizing" after #4303 gets merged - put a watch on it.

TomMalbran · 2013-08-06T04:17:00Z

Sure. With box-sizing: border-box the border and padding aren't taken into account with the width. So instead of having a width = width + padding + border + margin, width = width + margin. Use .sane-box-model mixin when using it.

TomMalbran · 2013-08-08T21:49:20Z

@zaggino #4303 has landed. So you can take another look at the fix.

I am thinking that the best would be to wrap the text searched and not anything else, but that might be harder to do. Maybe splitting the summary text and place it into different elements in the summary template...

zaggino · 2013-08-09T01:14:52Z

Works for me, just CSS changes.

TomMalbran · 2013-08-09T01:19:16Z

What would happen if there are more than 100 results and the pagination stuff is added to the summary? Is hard to happen but still possible.

zaggino · 2013-08-09T01:23:34Z

You're right, it's messed up, I'll fix that in a moment or two.

zaggino · 2013-08-09T01:58:47Z

.first-page, .prev-page, .next-page, .last-page icons are inline-block positioned so they are ignoring the text-overflow: ellipsis ... don't really know what to do with this without modifying too many things, not that experienced with CSS. Any ideas?

TomMalbran · 2013-08-09T02:15:08Z

I was thinking of trying to use flex-box to make it work like a table (modifying the template if required to add any additional div), and only have overflow over the text. But I have to try if that works.

zaggino · 2013-08-09T04:35:24Z

Tough one - at least for me, but finally got it working.

TomMalbran · 2013-08-09T04:46:05Z

Awesome. The results looks great.

If we split the summary into 3 parts (before, text, after) we could make it look like:
"long test that we want to..." found - Over 500 matches in 100 files (paste pagination here).
Where before = ", text = long test that we want to find in this project and after = found - Over 500 matches in 100 files.

Making only the searched text overflow with ellipsis.

zaggino · 2013-08-09T05:05:49Z

Great idea. Here it is.

I will probably need to fix translations too as one parameter was moved out of the string.

TomMalbran · 2013-08-09T05:13:32Z

Nice. Yes, I think we should just change the string, instead of the template in case a translation needs to change the order. Replacing the string to the next one and reversing the other changes (except the CSS) in the last commit should be better.

<div class="fixed-col">"</div><div class="contracting-col">{4}</div><div class="fixed-col">found {5} — {0} {1} in {2} {3}</div>"

zaggino · 2013-08-09T05:21:02Z

Wasn't sure about putting HTML into strings, ok sure thing.

TomMalbran · 2013-08-09T05:23:05Z

Yes, is ugly. Maybe we could use 3 strings, instead of harcoding the ".

TomMalbran · 2013-08-09T05:23:57Z

Actually, add the last " to the current string and have a string for the first ".

zaggino · 2013-08-09T05:31:05Z

I'm quite lost 😆 like this?

TomMalbran · 2013-08-09T05:39:16Z

Sorry. I changed my mind, since it does look ugly to place the html in the strings and I don't think we need to here.

So for the template we could have:

<div class="fixed-col">{{Strings.FIND_IN_FILES_PRE_TITLE}}</div>
<div class="contracting-col">{{query}}</div>
<div class="fixed-col">{{{summary}}}</div>
{{#hasPages}}
...

And the strings would be:

FIND_IN_FILES_PRE_TITLE:  "\"";
FIND_IN_FILES_TITLE: "\" found {4} &mdash; {0} {1} in {2} {3}";

Summary would be the formatted title string.

zaggino · 2013-08-09T07:37:56Z

No worries @TomMalbran , done.

TomMalbran · 2013-08-09T07:42:57Z

src/styles/brackets_mixins.less

+    display: flex;
+    flex-direction: row;
+}
+.flex-col(@grow: 0, @shrink: 1, @basis: auto) {


I think that flex-item is a better name for this mixin

TomMalbran · 2013-08-09T07:46:21Z

It looks great. I just added a few minor things.

zaggino · 2013-08-09T08:01:23Z

Ok, fixed all that.

TomMalbran · 2013-08-09T08:13:39Z

src/styles/brackets_mixins.less

@@ -21,6 +21,19 @@

 /* Brackets mixins */

+/* Helpers for working with flex layouts */
+/* see: https://developer.mozilla.org/en-US/docs/Web/CSS/flex */
+.flex-item(@direction: row) {


This one should be flex-box as before since is applied to the container.

zaggino · 2013-08-09T08:57:01Z

Sorry I messed that up, should be ok now.

TomMalbran · 2013-08-09T08:59:12Z

No problem. It looks great now. Will still have to wait for the next sprint to merge it. So some time next week.

zaggino · 2013-08-09T09:03:50Z

Ok great, let me know if anything will be needed from me.

zaggino · 2013-08-16T05:37:19Z

@TomMalbran should be ok to merge now ?

TomMalbran · 2013-08-22T05:16:58Z

I don't think there should be a min width, I just think that we shouldn't fix UI problems after a certain min width.

larz0 · 2013-08-22T05:17:48Z

@zaggino yes, see #998

TomMalbran · 2013-08-22T05:21:36Z

Lets merge this PR then?

zaggino · 2013-08-22T05:36:32Z

Sure, or I can merge master and fix also 4870 here if you want, can spare few minutes for that.

TomMalbran · 2013-08-22T05:38:14Z

If you have time, sure. If not, you can create a new PR for #4870.

zaggino · 2013-08-22T06:41:17Z

@TomMalbran can you review?

TomMalbran · 2013-08-22T06:47:34Z

I'll test how it looks now, code looks good.

We probably shouldn't be using IDs for the summaries, but that is another issue...

Want to also fix #4869 to? Since you are changing the strings and should be really easy to do?

zaggino · 2013-08-22T07:01:24Z

Sure, moment. I can remove those ID's too.

TomMalbran · 2013-08-22T07:36:34Z

I can't see the replace button...

I also found another problem in Find in Files when you find stuff in a scope with a really long path. I guess the path should contract too.

zaggino · 2013-08-22T08:03:36Z

Also noticed the replace button, fixed now.

zaggino · 2013-08-22T08:08:02Z

Test and review the current version please, it works fine with me. I'd rather not play with any more stuff here :)

TomMalbran · 2013-08-22T08:18:12Z

src/search/FindReplace.js

-            StringUtils.htmlEscape(replaceWhat.toString()),
-            StringUtils.htmlEscape(replaceWith.toString()),
-            results.length,
+            resultsString,


Do it like is done in find in files, so you can use matches here for consistency.

var summary = StringUtils.format( Strings.FIND_REPLACE_TITLE, results.length (results.length > 1) ? Strings.FIND_IN_FILES_MATCHES : Strings.FIND_IN_FILES_MATCH, results.length >= FIND_REPLACE_MAX ? Strings.FIND_IN_FILES_MORE_THAN : "" );

Which means that you need to add an extra {2} in FIND_REPLACE_TITLE, which could have the name FIND_REPLACE_POST_TITLE, to continue in the naming sequence you used.

You will never get 0 matches, since the modal bar collapses before you can click on any button.

TomMalbran · 2013-08-22T08:29:02Z

And there is still the issue with long scope labels. In the Brackets source go to src/extensibility/node_modules/unzip/node_modules/fstream/node_modules/mkdirp/test/umask_sunc.js right click on the file and use find in.

The search text contracts, but the scope label doesn't and it should for this extreme cases. You will have to move the scope label to the template and divide the Find in Files title one more time.

zaggino · 2013-08-22T11:36:02Z

@TomMalbran that last mentioned case is now ok. I couldn't make it to preserve spaces so I had to use   to actually show them.

TomMalbran · 2013-08-22T21:08:33Z

src/nls/root/strings.js

-    "FIND_IN_FILES_TITLE"               : "\"{4}\" found {5} &mdash; {0} {1} in {2} {3}",
+    "FIND_IN_FILES_PRE_TITLE"           : "\"",
+    "FIND_IN_FILES_MID_TITLE"           : "\" found",
+    "FIND_IN_FILES_TITLE"               : "&mdash; {0} {1} in {2} {3}",


Rename this to FIND_IN_FILES_POST_TITLE and FIND_REPLACE_POST_TITLE or do you think that FIND_REPLACE_TITLE_PART1, FIND_REPLACE_TITLE_PART2, FIND_REPLACE_TITLE_PART3 can be more clear? If is we should use the same format on both.

TomMalbran · 2013-08-22T21:09:59Z

Just 2 last minor Nits, and you can rebase before merging :)
Thanks for sticking with this.

Rebased: Fix for issue 4602. Fix for issue 4602. Fixed overflow when pagination is present. Little fix for no-pagination case, so there's no space reserved for pagination. Only searched text is contracting now with ellipsis. Moved layout back to strings. Little more refactoring. Few more changes according code review. Few more changes according code review. #2 Fixes #4870 Fixed duplicate ID Remove IDs used for summaries. Fixes #4869 Refactored according to code-review. Fixes case when scope is a long filename. Minor refactoring

zaggino · 2013-08-22T22:17:18Z

Done, why squashed commit shows 17 days ago and not current time? What did I do wrong? :)

TomMalbran · 2013-08-22T22:26:10Z

It is showing the day of the first commit. There is a way for it to show as the last commit, but I am not sure how.

Looks good. Merging :)

Fix for issue 4602.

njx · 2013-08-22T23:13:35Z

I don't know of a way to update the date during an interactive rebase, but you could do another (non-interactive) rebase afterwards and specify --ignore-date. (There's also a way to use git commit --amend: see the last ansewr on http://stackoverflow.com/questions/454734/how-can-one-change-the-timestamp-of-an-old-commit-in-git)

That said, if you're just squashing into a single commit, it might be easier to just do a git merge --squash into a new branch. When you merge that way, it doesn't actually create the commit right away (it just sets up everything in your working directory properly so you can do a commit). So, when you create the commit, it will have the right date.

zaggino · 2013-08-23T00:54:01Z

thanks @njx , I'll play with the --ignore-date next time :-)

TomMalbran reviewed Aug 9, 2013
View reviewed changes

TomMalbran reviewed Aug 22, 2013
View reviewed changes

ghost assigned TomMalbran Aug 22, 2013

TomMalbran reviewed Aug 22, 2013
View reviewed changes

TomMalbran added a commit that referenced this pull request Aug 22, 2013

Merge pull request #4661 from zaggino/issue4602

8dc29bd

Fix for issue 4602.

TomMalbran merged commit 8dc29bd into adobe:master Aug 22, 2013

zaggino deleted the issue4602 branch August 22, 2013 23:23

peterflynn mentioned this pull request Aug 29, 2014

[Japanese][FindReplace]: FindReplace title shows incorrectly if only replace one character on Japanese OS #8883

Open

This was referenced Aug 29, 2021

[CLOSED] Fix for issue 4602. brackets-archive/bracketsIssues#4305

Open

[CLOSED] [Core][Search replace panel]: The search replace panel will get truncated if the find-replace-title is too long. brackets-archive/bracketsIssues#4501

Open

Fix for issue 4602. #4661

Fix for issue 4602. #4661

Conversation

zaggino commented Aug 6, 2013

zaggino commented Aug 6, 2013

TomMalbran commented Aug 6, 2013

zaggino commented Aug 6, 2013

TomMalbran commented Aug 6, 2013

TomMalbran commented Aug 8, 2013

zaggino commented Aug 9, 2013

TomMalbran commented Aug 9, 2013

zaggino commented Aug 9, 2013

zaggino commented Aug 9, 2013

TomMalbran commented Aug 9, 2013

zaggino commented Aug 9, 2013

TomMalbran commented Aug 9, 2013

zaggino commented Aug 9, 2013

TomMalbran commented Aug 9, 2013

zaggino commented Aug 9, 2013

TomMalbran commented Aug 9, 2013

TomMalbran commented Aug 9, 2013

zaggino commented Aug 9, 2013

TomMalbran commented Aug 9, 2013

zaggino commented Aug 9, 2013

TomMalbran Aug 9, 2013

Choose a reason for hiding this comment

TomMalbran commented Aug 9, 2013

zaggino commented Aug 9, 2013

TomMalbran Aug 9, 2013

Choose a reason for hiding this comment

zaggino commented Aug 9, 2013

TomMalbran commented Aug 9, 2013

zaggino commented Aug 9, 2013

zaggino commented Aug 16, 2013

TomMalbran commented Aug 22, 2013

larz0 commented Aug 22, 2013

TomMalbran commented Aug 22, 2013

zaggino commented Aug 22, 2013

TomMalbran commented Aug 22, 2013

zaggino commented Aug 22, 2013

TomMalbran commented Aug 22, 2013

zaggino commented Aug 22, 2013

TomMalbran commented Aug 22, 2013

zaggino commented Aug 22, 2013

zaggino commented Aug 22, 2013

TomMalbran Aug 22, 2013

Choose a reason for hiding this comment

TomMalbran Aug 22, 2013

Choose a reason for hiding this comment

TomMalbran commented Aug 22, 2013

zaggino commented Aug 22, 2013

TomMalbran Aug 22, 2013

Choose a reason for hiding this comment

TomMalbran commented Aug 22, 2013

zaggino commented Aug 22, 2013

TomMalbran commented Aug 22, 2013

njx commented Aug 22, 2013

zaggino commented Aug 23, 2013