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

Merged
merged 1 commit into from Aug 6, 2014

Projects

None yet

3 participants

@MiguelCastillo
Member

Loading theme files directly instead of a package breaks old themes; e.g. vanilla codemirror themes. The issue is caused by adding the file extension as part of the theme name. This logic was to handle edge cases when themes had the same filename but different extensions, but this edge case should really be resolved by giving files a better name. This was important in earlier impls of themes that provided an interface to load all the theme files in a diretory (loadDirectory) but that interface was removed a while ago.

@dangoor dangoor self-assigned this Aug 6, 2014
@dangoor dangoor merged commit d7d394e into adobe:master Aug 6, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@dangoor
Contributor
dangoor commented Aug 6, 2014

Thanks for the fix!

@MarcelGerber MarcelGerber commented on the diff Aug 6, 2014
src/view/ThemeManager.js
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, '-');
@MarcelGerber
MarcelGerber Aug 6, 2014 Member

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

@MarcelGerber
MarcelGerber Aug 6, 2014 Member

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

@MiguelCastillo
MiguelCastillo Aug 6, 2014 Member

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

@dangoor
dangoor Aug 6, 2014 Contributor

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

@dangoor
dangoor Aug 6, 2014 Contributor

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

@MiguelCastillo
MiguelCastillo Aug 6, 2014 Member

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

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