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

setFile #2128

Merged
merged 7 commits into from
Jul 1, 2019
Merged

setFile #2128

merged 7 commits into from
Jul 1, 2019

Conversation

LianaHus
Copy link
Collaborator

@LianaHus LianaHus commented Jun 28, 2019

@LianaHus LianaHus changed the title "save as" saves a copy with .vy extention and a number setFile Jun 28, 2019
src/app/files/fileManager.js Outdated Show resolved Hide resolved
</div>`
return yo`
<div class="container ml-1">
<button class="btn btn-primary btn-sm m-1" onclick=${(e) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Create a closure (local function) instead of writing the function inside the onclick tag

<div class="container ml-1">
<button class="btn btn-primary btn-sm m-1" onclick=${(e) => {
reject = false
e.target.innerHTML = 'Accepted'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should try to adopt a reactive way of programming if possible. In this case you would create a variable accepted, and call yo.update. Don't change it inside this PR, I'll create an issue for that.

}}>
Accept
</button>
<button class="btn btn-primary btn-sm m-1" onclick=${(e) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, create a closure.

}}>
Cancel
</button>
<button class="btn btn-primary btn-sm m-1" onclick=${(e) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same

src/app/ui/tooltip.js Show resolved Hide resolved
@LianaHus LianaHus merged commit 30aa20f into master Jul 1, 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
Development

Successfully merging this pull request may close these issues.

setFile's prompt has a frustrating behaviour.
2 participants