Skip to content
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

compiler tab style fixes. language selector ui behaviour #2108

Merged
merged 7 commits into from
Jun 26, 2019
Merged

Conversation

LianaHus
Copy link
Collaborator

@LianaHus LianaHus commented Jun 24, 2019

@LianaHus
Copy link
Collaborator Author

#2087

@LianaHus LianaHus changed the title compiler tab style fixes. same padding for all items compiler tab style fixes. language selector ui behaviour Jun 24, 2019
@yann300 yann300 force-pushed the master_li branch 4 times, most recently from 0cbe0d2 to 0752b06 Compare June 25, 2019 11:58
@@ -294,6 +305,16 @@ class CompilerContainer {
}
}

_updateLanguageSelector () {
if (this._retriveVersion() < '0.5.7') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

how this can work? you should probably use semver here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

:) it works. ok I'll look at it

Copy link
Collaborator

@yann300 yann300 left a comment

Choose a reason for hiding this comment

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

we should use semver for version comparison

@yann300
Copy link
Collaborator

yann300 commented Jun 25, 2019

some infos about semver https://www.npmjs.com/package/semver

@@ -240,7 +248,9 @@ class CompilerContainer {

onchangeLanguage (event) {
this.compileTabLogic.setLanguage(event.target.value)
this.compile()
Copy link
Collaborator

Choose a reason for hiding this comment

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

all the other on* method, just trigger a new compilation. Might be more consistent to to keep the same behavior here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

but there was a issue here. build shouldn't be triggered if there is no autocompile an selected file

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

or I can put the check in the compile() function

Copy link
Collaborator

Choose a reason for hiding this comment

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

but there was a issue here. build shouldn't be triggered if there is no autocompile an selected file

This should done in another PR and all the on* function should be updated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok will create a new PR

will be added in new PR
@yann300
Copy link
Collaborator

yann300 commented Jun 26, 2019

Click on Solidity env (from the front page), the selected compiler is 0.5.1, the dropdown list is still enabled. I guess it should not be

@LianaHus LianaHus merged commit 1afc556 into master Jun 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants