Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Changes to allow backward compatibility with plain ole CodeMirror themes #8673

Merged
merged 1 commit into from
Aug 6, 2014
Merged

Changes to allow backward compatibility with plain ole CodeMirror themes #8673

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions src/view/ThemeManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,13 +79,11 @@ define(function (require, exports, module) {
options = options || {};
var fileName = file.name;

// The name is used to map the loaded themes to the list in the settings dialog. So we want
// a unique name if one is not provided. This is particularly important when loading just
// files where there is no other way to feed in meta data to provide unique names. Say, there
// is a theme1.css and a theme1.less that are entirely different themes...
// Portip: If no options.name or options.title is provided, then we will rely solely on
// the name of the file to be unique

this.file = file;
this.name = options.name || (options.title || fileName).toLocaleLowerCase().replace(/[\W]/g, '-');
this.name = options.name || (options.title || fileName.replace(/.[\w]+$/gi, '')).toLocaleLowerCase().replace(/[\W]/g, '-');
Copy link
Contributor

Choose a reason for hiding this comment

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

Well, didn't you mean replace(/\.[\w]+$/gi, '') (escaped dot)?
You don't need the i option, btw.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, and what about double extensions like .css.min?
We should maybe add an unit test for that, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had escaped initially, but when I tested it it didn't make a difference. Check it out http://regex101.com/r/iI6oQ0/1

Copy link
Contributor

Choose a reason for hiding this comment

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

Err, yeah, escaping the dot is required because "indexhtml" matches the regex as it stands now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Of course, these are edge cases for things that aren't hit in core usage of the feature anyhow...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to make sure the stuff works as expected though so I am make another PR with the dot escaped. Good catch guys!

this.displayName = options.title || toDisplayName(fileName);
this.dark = options.theme !== undefined && options.theme.dark === true;
}
Expand Down
6 changes: 3 additions & 3 deletions test/spec/ThemeManager-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ define(function (require, exports, module) {

waitsForDone(promise, "theme with scrollbar and other css", 5000);
});

it("should extract scrollbars from a theme with only scrollbars", function () {
var themeFile = FileSystem.getFileForPath(testFilePath + "/simple-scrollbars.css");
var promise = FileUtils.readAsText(themeFile).done(function (content) {
Expand All @@ -83,7 +83,7 @@ define(function (require, exports, module) {

waitsForDone(promise, "theme with only scrollbars", 5000);
});

it("should be fine with an empty theme", function () {
var themeFile = FileSystem.getFileForPath(testFilePath + "/empty.css");
var promise = FileUtils.readAsText(themeFile).done(function (content) {
Expand All @@ -100,7 +100,7 @@ define(function (require, exports, module) {
describe("Load themes", function () {
it("should load a theme from a single CSS file", function () {
var promise = ThemeManager.loadFile(testFilePath + "/scrollbars.css").done(function (theme) {
expect(theme.name).toEqual("scrollbars-css");
expect(theme.name).toEqual("scrollbars");
expect(theme.displayName).toEqual("Scrollbars");
});

Expand Down