New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Project View Search Button #509

Merged
merged 10 commits into from Oct 2, 2015

Conversation

Projects
None yet
5 participants
@thisisnian
Contributor

thisisnian commented Aug 25, 2015

This is to create a 'Find' button so that users are able to press Find instead of having to press enter.
This is to improve user friendiness, instead of having to guess enter = find.

This is my first dabble in coffee, code review appreciated.

Please see #297

Cheers,


Added by @simurai

Before:

screen shot 2015-09-22 at 10 48 29 pm

After:

screen shot 2015-09-22 at 10 46 07 pm

Once this gets merged, also merge One dark/light PRs atom/one-dark-ui#91 + atom/one-light-ui#35.

Project View Search Button
This is to add a find button in the project search for better UI
friendiness

Please see #297
@mertkahyaoglu

This comment has been minimized.

Show comment
Hide comment
@mertkahyaoglu

mertkahyaoglu Aug 25, 2015

IMO, just pressing enter is fast and very user friendly 😁 because obviously the main action is to search something. Let's see what others think.

Other editors; VS Code has next and previous arrows for searching in current buffer and no button for searching in project. Sublime has 'find' and 'find next' buttons for current buffer and 'find' button for project.

About the code, the only thing bothers me is additional line breaks.

mertkahyaoglu commented Aug 25, 2015

IMO, just pressing enter is fast and very user friendly 😁 because obviously the main action is to search something. Let's see what others think.

Other editors; VS Code has next and previous arrows for searching in current buffer and no button for searching in project. Sublime has 'find' and 'find next' buttons for current buffer and 'find' button for project.

About the code, the only thing bothers me is additional line breaks.

@thisisnian

This comment has been minimized.

Show comment
Hide comment
@thisisnian

thisisnian Aug 25, 2015

Contributor

Well there have been some users (myself included) that wasn't sure how to search, either this or even an 'Enter to Search' tooltip or something.

In any case, users can still press the enter button, whatever floats your boat.

I've just fixed the formatting.

Contributor

thisisnian commented Aug 25, 2015

Well there have been some users (myself included) that wasn't sure how to search, either this or even an 'Enter to Search' tooltip or something.

In any case, users can still press the enter button, whatever floats your boat.

I've just fixed the formatting.

@thisisnian

This comment has been minimized.

Show comment
Hide comment
@thisisnian

thisisnian Aug 25, 2015

Contributor

thanks must have missed that one.

Contributor

thisisnian commented Aug 25, 2015

thanks must have missed that one.

@50Wliu 50Wliu added the needs-review label Aug 26, 2015

@simurai

This comment has been minimized.

Show comment
Hide comment
@simurai

simurai Aug 26, 2015

Member

How about changing the button widths a bit so that the inputs line up? Something like:

screen shot 2015-08-26 at 4 50 19 pm

I can help out with the styling if you like it.

Member

simurai commented Aug 26, 2015

How about changing the button widths a bit so that the inputs line up? Something like:

screen shot 2015-08-26 at 4 50 19 pm

I can help out with the styling if you like it.

thisisnian added some commits Aug 26, 2015

Change button width for find in project
Changed the replace all button width to span across so that the
find/replace inputs line up
formatting
removed unnecessary space
@thisisnian

This comment has been minimized.

Show comment
Hide comment
@thisisnian

thisisnian Aug 26, 2015

Contributor

@simurai Got it like the screenshot above, and I've got it built in Linux. However it fails Travis CI. I guess I'm doing something wrong?

Contributor

thisisnian commented Aug 26, 2015

@simurai Got it like the screenshot above, and I've got it built in Linux. However it fails Travis CI. I guess I'm doing something wrong?

@thisisnian thisisnian closed this Aug 27, 2015

@thisisnian thisisnian reopened this Aug 27, 2015

@simurai

This comment has been minimized.

Show comment
Hide comment
@simurai

simurai Aug 27, 2015

Member

I think the check fails because project-find got renamed to find-and-replace.

Edit: Made a PR where the project-find class name is changed back and the buttons resized: thisisnian#1

Note to self: If this gets merged, the One themes need to be updated too.

Member

simurai commented Aug 27, 2015

I think the check fails because project-find got renamed to find-and-replace.

Edit: Made a PR where the project-find class name is changed back and the buttons resized: thisisnian#1

Note to self: If this gets merged, the One themes need to be updated too.

@thisisnian

This comment has been minimized.

Show comment
Hide comment
@thisisnian

thisisnian Aug 28, 2015

Contributor

I reverted the failing changes on my local instance, this is what I got

image

I'm not pretty sure where the property (boxed in red) comes from. Looks like it may be from another package as the one defined in the styles gets overwritten. Has to be more than 10em otherwise the buttons are too small.

Gah I feel so stupid, must be something really simple that I'm missing.

Contributor

thisisnian commented Aug 28, 2015

I reverted the failing changes on my local instance, this is what I got

image

I'm not pretty sure where the property (boxed in red) comes from. Looks like it may be from another package as the one defined in the styles gets overwritten. Has to be more than 10em otherwise the buttons are too small.

Gah I feel so stupid, must be something really simple that I'm missing.

@simurai

This comment has been minimized.

Show comment
Hide comment
@simurai

simurai Aug 29, 2015

Member

I'm not pretty sure where the property (boxed in red) comes from. Looks like it may be from another package as the one defined in the styles gets overwritten.

Yes, right. That comes from the One dark theme. If you switch to another theme (like Atom dark), then it should look fine. I'll make another PR for the One dark theme to fix it. It needs the ems so that font sizing can be changed.

Tip: In case you'd like to see where the styles come from, you can click the <style>...</style>, then it should highlight the less file.

screen shot 2015-08-29 at 12 20 12 pm
screen shot 2015-08-29 at 12 20 44 pm

Member

simurai commented Aug 29, 2015

I'm not pretty sure where the property (boxed in red) comes from. Looks like it may be from another package as the one defined in the styles gets overwritten.

Yes, right. That comes from the One dark theme. If you switch to another theme (like Atom dark), then it should look fine. I'll make another PR for the One dark theme to fix it. It needs the ems so that font sizing can be changed.

Tip: In case you'd like to see where the styles come from, you can click the <style>...</style>, then it should highlight the less file.

screen shot 2015-08-29 at 12 20 12 pm
screen shot 2015-08-29 at 12 20 44 pm

@thisisnian

This comment has been minimized.

Show comment
Hide comment
@thisisnian

thisisnian Aug 29, 2015

Contributor

Ah I knew it came from there, however I was just unsure as it would mean potentially not just one-dark theme that needs changing, however anyone with a custom theme that didn't have the correct css attributes (could be many in the future)

Thanks for checking over it.

Contributor

thisisnian commented Aug 29, 2015

Ah I knew it came from there, however I was just unsure as it would mean potentially not just one-dark theme that needs changing, however anyone with a custom theme that didn't have the correct css attributes (could be many in the future)

Thanks for checking over it.

@simurai

This comment has been minimized.

Show comment
Hide comment
@simurai

simurai Sep 3, 2015

Member

I was just unsure as it would mean potentially not just one-dark theme that needs changing, however anyone with a custom theme that didn't have the correct css attributes (could be many in the future)

Typically, themes don't have to bother about styling those buttons. Unless they wanna customize it. The One themes override some of the px values to make it possible to change the font size so the whole UI can be resized. But that shouldn't be the concern of this package.

It is a bit of a problem when people fork the One themes, since those themes would need a fix too.

Member

simurai commented Sep 3, 2015

I was just unsure as it would mean potentially not just one-dark theme that needs changing, however anyone with a custom theme that didn't have the correct css attributes (could be many in the future)

Typically, themes don't have to bother about styling those buttons. Unless they wanna customize it. The One themes override some of the px values to make it possible to change the font size so the whole UI can be resized. But that shouldn't be the concern of this package.

It is a bit of a problem when people fork the One themes, since those themes would need a fix too.

@simurai

This comment has been minimized.

Show comment
Hide comment
@simurai

simurai Sep 25, 2015

Member

@benogle How do you like this change of adding a Find button to project search? I quite like it:

  • Better discovery for beginners
  • More consistent with the find in buffer.
Member

simurai commented Sep 25, 2015

@benogle How do you like this change of adding a Find button to project search? I quite like it:

  • Better discovery for beginners
  • More consistent with the find in buffer.
@benogle

This comment has been minimized.

Show comment
Hide comment
@benogle

benogle Sep 25, 2015

Contributor

I think it makes sense. It would be nice if there were a test. Other than that, the code looks fine.

Contributor

benogle commented Sep 25, 2015

I think it makes sense. It would be nice if there were a test. Other than that, the code looks fine.

benogle added a commit that referenced this pull request Oct 2, 2015

@benogle benogle merged commit a416d0b into atom:master Oct 2, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment