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

Fix autocomplete icon #18016

Merged
merged 1 commit into from Sep 12, 2018

Conversation

Projects
None yet
4 participants
@simurai
Member

simurai commented Sep 11, 2018

Description of the Change

This fixes the icons in the autocomplete popover.

Before After
image image

Alternate Designs

The selector used to win in specificity is pretty ugly, but we can't really change the class names because other packages and themes use it too.

Why Should This Be In Core?

It's a bundled package

Benefits

Icon's background doesn't look broken anymore.

Possible Drawbacks

Even harder to override

Verification Process

  1. Open a file
  2. Start typing so that the autocomplete popover shows up
  3. Verify that the icon's background looks ok

Applicable Issues

None, pointed out in Slack by @maxbrunsfeld

@simurai simurai added the in progress label Sep 11, 2018

@thomasjo

This comment has been minimized.

Show comment
Hide comment
@thomasjo

thomasjo Sep 11, 2018

Member

How did this regression sneak in? Maybe we should experiment with some (simple) means of preventing this kind of breakage in the future — ideally some type of automated test. If not, we should at least have some manual smoke testing process that should pass anytime we touch GUI bits.

Member

thomasjo commented Sep 11, 2018

How did this regression sneak in? Maybe we should experiment with some (simple) means of preventing this kind of breakage in the future — ideally some type of automated test. If not, we should at least have some manual smoke testing process that should pass anytime we touch GUI bits.

@thomasjo

This comment has been minimized.

Show comment
Hide comment
@thomasjo

thomasjo Sep 11, 2018

Member

(Automated) regression/smoke testing will also catch stuff that breaks whenever we upgrade Electron/Chromium, which could otherwise easily sneak past us. Not sure how frequently this happens, but added an "bonus" nonetheless.

Member

thomasjo commented Sep 11, 2018

(Automated) regression/smoke testing will also catch stuff that breaks whenever we upgrade Electron/Chromium, which could otherwise easily sneak past us. Not sure how frequently this happens, but added an "bonus" nonetheless.

@simurai

This comment has been minimized.

Show comment
Hide comment
@simurai

simurai Sep 11, 2018

Member

How did this regression sneak in?

By trying to fix this issue with this PR. Reminds me of this joke. 😂

ideally some type of automated test

I think there are some testing frameworks that do "visual diffing". Take before and after screenshots and test if pixels changed. No idea how easy it would be to add something like that.

manual smoke testing process

Yeah, have a list of things to manually click through in the UI. It wouldn't catch everything and there are probably community packages that we constantly break, but at least for the core UI.

Another solution would be to make components more robust and independant. Like in this PR's case, the CSS class .list-group gets used by the autocomplete popover, but also by the tree-view, command palette, search results and probably other places and also community packages. They are all "lists" but still different enough that it would make more sense to style them independently so they wouldn't need overriding for each case. But it's kinda too late because themes, packages and user's styles.less already use the .list-group class and we have to keep it till a next major version of Atom.

So yeah, maybe back to a smoke test?

Member

simurai commented Sep 11, 2018

How did this regression sneak in?

By trying to fix this issue with this PR. Reminds me of this joke. 😂

ideally some type of automated test

I think there are some testing frameworks that do "visual diffing". Take before and after screenshots and test if pixels changed. No idea how easy it would be to add something like that.

manual smoke testing process

Yeah, have a list of things to manually click through in the UI. It wouldn't catch everything and there are probably community packages that we constantly break, but at least for the core UI.

Another solution would be to make components more robust and independant. Like in this PR's case, the CSS class .list-group gets used by the autocomplete popover, but also by the tree-view, command palette, search results and probably other places and also community packages. They are all "lists" but still different enough that it would make more sense to style them independently so they wouldn't need overriding for each case. But it's kinda too late because themes, packages and user's styles.less already use the .list-group class and we have to keep it till a next major version of Atom.

So yeah, maybe back to a smoke test?

@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Sep 11, 2018

Contributor

The package migration is paying off; it's nice to see both of these changes in one PR.

Thanks for fixing this!

Contributor

maxbrunsfeld commented Sep 11, 2018

The package migration is paying off; it's nice to see both of these changes in one PR.

Thanks for fixing this!

@simurai

This comment has been minimized.

Show comment
Hide comment
@simurai

simurai Sep 12, 2018

Member

The package migration is paying off; it's nice to see both of these changes in one PR.

Yes, totally. 💯 It's super nice to not have to make two "identical" PRs.

We might could go even further and have a "master" theme from where other color variations could import the styles from. I'll have to test that one day.

Member

simurai commented Sep 12, 2018

The package migration is paying off; it's nice to see both of these changes in one PR.

Yes, totally. 💯 It's super nice to not have to make two "identical" PRs.

We might could go even further and have a "master" theme from where other color variations could import the styles from. I'll have to test that one day.

@simurai

This comment has been minimized.

Show comment
Hide comment
@simurai

simurai Sep 12, 2018

Member

The failing check seems to be unrelated since master is already failing with the same error.

Member

simurai commented Sep 12, 2018

The failing check seems to be unrelated since master is already failing with the same error.

@simurai simurai merged commit e865182 into master Sep 12, 2018

2 of 3 checks passed

Atom Pull Requests #20180911.2 failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@simurai simurai deleted the sm/auto-complete branch Sep 12, 2018

@defunkt defunkt removed the in progress label Sep 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment