Skip to content

[FEATURE] Support theme (temporary work before building theme plugin)#751

Merged
hwshim merged 3 commits intowebida:masterfrom
kyungmi:theme
Nov 25, 2015
Merged

[FEATURE] Support theme (temporary work before building theme plugin)#751
hwshim merged 3 commits intowebida:masterfrom
kyungmi:theme

Conversation

@kyungmi
Copy link
Contributor

@kyungmi kyungmi commented Nov 24, 2015

[DESC.]

  • Using theme.apply(), image resources can be loaded by its sub path on theme set directories.
  • For instance, it was applied at two places on source codes(Help > About dialog, preference toolbar icon).

[DESC.]
- Using `theme.apply()`, image resources can be loaded by its sub path on theme set directories.
- For instance, it was applied at two places on source codes(Help > About dialog, preference toolbar icon).
@kyungmi kyungmi added this to the webida v 1.6.0 milestone Nov 24, 2015
@kyungmi
Copy link
Contributor Author

kyungmi commented Nov 24, 2015

@changgyun-woo You can use this theme util after this codes will be merged LIKES BELOW.

some plugin.json

... 
icons: "<%= themePath %>/images/icons/toolbar_preference.png"
...

Or, It can be also used in js file

require(['webida-lib/theme'], function(theme) {
...
var themeAppliedTemplate = theme.apply('<img src="<%=themePath%>/images/logo.png">');
// '<img src="/apps/ide/src/styles/theme/webida-light/images/logo.png">'
var themeAppliedImageUrl = theme.apply('<%=themePath%>/images/icon/someIcon.png'); 
// '/apps/ide/src/styles/theme/webida-light/images/icon/someIcon.png'
var themeName = theme.apply('<%=theme%> is applied!'); 
// 'webida-light is applied!'
...
});

@changgyun-woo Can you remind me about more things other than 'toolbar icons' that should be applied theme?

Copy link
Member

Choose a reason for hiding this comment

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

'webida/util/' will be split into it's own repository as bower package soon.
But this code refers to 'webida-lib/app-config' directly.
This code make it difficult to split out 'webida/util/'
This dependency should be refactored.
Then this commit might be merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where is the proper place for common lib for only IDE?
As I know there are some modules only related with IDE in common dir.

Copy link
Member

Choose a reason for hiding this comment

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

The above code is not common because it refers to 'app-config.js'
If the code could be common, then the code can be placed 'webida/util/'
You can consider use just _.template() rather than wrapping it.
Or whatever better solution except for this kind.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

app-config is also common configuration file for IDE. and any plugins can refer the app-config.

Copy link
Member

Choose a reason for hiding this comment

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

Do you really think this way is right?
"some modules related with IDE in common dir" should be refactored
when I split out webida/util as a bower package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

그래서 제 첫번째 질문이 commob/util 말고 어디에 넣는게 맞느냐라고 여쭤본겁니다. 제가 작성한 코드가 ide안에서만 종속성을 갖는 코드가 맞기 때문에 어디로 옮겨야 할지 모르겠네요. 일단 apply의 결과물이 ide에 상대적인 패스로 나올수 밖에 없어요. 그리고 임시용 코드이기 때문여 더 구조를 복잡하게 만들고 싶지 않았습니다. 의존성을 당연히 가져야 되는 코드기 때문에 그렇게 한 것이지 다른 의도는 없었습니다. 좋은 방법이 있다면 알려주셔요. 적용하겠습니다.

Copy link
Member

Choose a reason for hiding this comment

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

lodash 를 랩핑한거라면 그냥 lodash를 사용하는 것이 이해하기도 쉽고 나아보입니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

중복 코드가 많이 생길텐데 괜찮을까요?

Copy link
Member

Choose a reason for hiding this comment

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

theme-manager.js 로 common/src/webida/ 디렉터리에 위치시키고 apply 라는 이름 보다는 parse 정도가 나아 보입니다. themeManager.parse('<%=theme%> applied') 이것도 일시적인 것이고 좀더 검토해서 theme 관리 플러그인으로 이동시킬지 1.7.0 이후에 생길 webida-runtime 디렉터리에 넣을지 검토가 필요합니다. 참고로 webida-runtime 디렉터리는 plugin-manager.js 등이 위치할 곳 입니다. 일단은 common/src/webida 로 이동 하면 될것 같습니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

네 이동시키겠습니다. 하지만 저는 parse 보다는 apply 가 의미상 더 좋은 것 같네요. 저 문자열을 파싱해서 뭘 하는건 아니니까요. 테마에 관련된 특정 값들을 어떤 패스 혹은 스트링에 적용한다는 의미에서 apply가 더 적합해 보입니다.

hwshim added a commit that referenced this pull request Nov 25, 2015
[FEATURE] Support theme (temporary work before building theme plugin)
@hwshim hwshim merged commit 02bb56f into webida:master Nov 25, 2015
@changgyun
Copy link
Contributor

@kyungmi I want to get the value for the theme to the index page.
If I get a theme name from the index page , it will be easier theme management.

ex) less.modifyVars ({"@theme": "webida-light"}, true);
Using Less in the Browser

  • The less I use in the bower?

@kyungmi kyungmi deleted the theme branch November 25, 2015 08:52
@kyungmi
Copy link
Contributor Author

kyungmi commented Nov 25, 2015

@changgyun-woo We reached a conclusion that using pre-build on less files on deploy or dev phase is better than dynamically compiling on loading time.
So, I'll write Gruntfile.js for this pre-build. Then you can use it on development also with grunt command.

@hwshim
Copy link
Member

hwshim commented Nov 26, 2015

Suggestion for using ClassName

@changgyun-woo
@kyungmi

In addition, I think the codes (in the plugin.json) like following

"toolbar": {
    "icons" : "./styles/theme/<%=theme%>/images/icons/toolbar_save.png",
    "tooltip" : "Save",
    "enabledOn" : "editor/dirty/current",
    "disabledOn" : "editor/clean/current"
}

should be changed like this stepwise.

"toolbar": {
    "class" : "toolbar_save",
    "tooltip" : "Save",
    "enabledOn" : "editor/dirty/current",
    "disabledOn" : "editor/clean/current"
}

If the higher className changed, the nested style will be automatically applied.

body.light .toolbar_save {
   background-image : url (~light button path~);
}
body.dark .toolbar_save {
   background-image : url (~dark button path~);
}

In short, I think we have to find a way to use 'className' rather than using 'whole path of resource'.

@changgyun
Copy link
Contributor

And override the css to the above method in the current WSDK.

Whether the topic is important to set the image in the json file?
Even if the above method I have it good.

I configure all the icons in a single image is thought to control the position.

@kyungmi
Copy link
Contributor Author

kyungmi commented Nov 27, 2015

@hwshim As I know, the configuration of toolbar has also the property named "iconClass" that can be replaced with current "icons" property. So, the way you suggesting can be easily applied to our system with no need to edit toolbar.js's source.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants