Skip to content
This repository has been archived by the owner on Oct 28, 2020. It is now read-only.

78 add platform styles #95

Merged
merged 1 commit into from
Mar 23, 2016
Merged

78 add platform styles #95

merged 1 commit into from
Mar 23, 2016

Conversation

kaganjd
Copy link
Contributor

@kaganjd kaganjd commented Mar 21, 2016

No description provided.

background-color: Highlight;
color: HighlightText;
background-image: linear-gradient(rgba(255,255,255,0.3), transparent);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This little bit in the diff:

screen shot 2016-03-21 at 8 37 29 am

Means that there's no newline at the end of the file. Would you mind adding that for all the CSS files you added?

@chuckharmston
Copy link
Contributor

This is really fantastic work. Great job, @kaganjd! If you wouldn't mind addressing the few nits above and squashing into one commit, I'd be happy to merge this.

@@ -0,0 +1,6 @@
@import 'style.css';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kaganjd I'm not sure we want to use '@import' because we'll need to manually unload the stylesheets when the addon is disabled/uninstalled.

Would you mind manually loading / unloading both the main style.css file and the platform-specific files?

@jaredhirsch jaredhirsch self-assigned this Mar 21, 2016
@jaredhirsch
Copy link
Member

@kaganjd Ok, getting close! Once the loading and unloading covers both stylesheets, we'll do a final syntax pass and land this thing. Nice work :-)

#universal-search-recommendation {
background-color: Highlight;
color: HighlightText;
}/n
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kaganjd Sorry, I didn't explain this correctly. I think you want to make sure your editor is inserting newlines at the end of files by default.

Here's a great article from Thoughtbot that explains the historical motivation for this convention, and how to configure common editors like vim and emacs: https://robots.thoughtbot.com/no-newline-at-end-of-file

@jaredhirsch
Copy link
Member

@kaganjd OK, we're nearly done, great job. This pass probably looks like a ton of comments, but I really just wanted to hit all the minor details in a single round. Once you've made these changes, it'll be good to land (though I'll take a last glance to be sure).

Please feel free to ask questions, and ping me when this is ready for a final review. :-)

#universal-search-recommendation.highlight {
background-color: Highlight;
color: HighlightText;
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kaganjd We actually don't want semicolons at the end of CSS blocks, we just want the closing bracket. Here's some discussion on MDN: https://developer.mozilla.org/en-US/docs/Web/CSS/Syntax#CSS_declarations_blocks

@jaredhirsch
Copy link
Member

@kaganjd Nice work! Remove the trailing semicolons from the CSS files, re-push, and I'll hit the big green button 🎉

@kaganjd
Copy link
Contributor Author

kaganjd commented Mar 22, 2016

cool! done, i think. fingers crossed

@jaredhirsch
Copy link
Member

@kaganjd Looks good! One more thing: you need to squash your 5 commits into a single commit, and reword the commit message. This shouldn't take too long:

  • git rebase -i HEAD~5 will open up interactive rebase mode, it'll initially look like this:

screen shot 2016-03-22 at 1 53 54 pm

- squash the commits together by leaving the 'pick' at the front of the top line, and for the 4 lines below it, replace the 'pick' with 's' (for squash):

screen shot 2016-03-22 at 1 54 36 pm

- save and exit out of the editor to rebase, git will jump to the command line while rebasing, then you'll jump back to an editor window that shows all the commit messages:

screen shot 2016-03-22 at 1 55 29 pm

I think the initial commit message was great, you just need to add a "Fixes #78" on a new line, something like this should work, note that present tense is used though:

screen shot 2016-03-22 at 2 01 35 pm

So, squash your commits, push once more, and I'll land the patch for you. Great work ^_^

Background reading, in case you're interested:

edited to linkify tbaggery link

@jaredhirsch jaredhirsch merged commit 2f4a141 into mozilla:master Mar 23, 2016
Create platform-specific stylesheets for Windows, OSX, and Linux. Edit universal-search.js to load the correct stylesheet for the platform.

Fixes #78.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants