Skip to content

Conversation

jstr14
Copy link
Contributor

@jstr14 jstr14 commented Apr 18, 2018

add disableScroll property to avoid scrolling views behind lightbox

showLightBox: {
type: Boolean,
default: true,
default: true
Copy link
Owner

Choose a reason for hiding this comment

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

I think you should keep the little comma there. We use Airbnb Javascript Style Guide

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your reply I will change it

},

data() {
data () {
Copy link
Owner

Choose a reason for hiding this comment

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

We use this rule

@pexea12
Copy link
Owner

pexea12 commented Apr 18, 2018

I have just commented on the changes of your pull request.

I would approve your pull request if you keep the style unchanged and change only what necessary.


disableScroll: {
type: Boolean,
default: false
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 this is a good improvement so we can turn on it by default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your comment, I change it :)


.no-scroll {
overflow-y: hidden;
}
Copy link
Owner

Choose a reason for hiding this comment

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

A little change: can you add a new line to the end of this file? Sorry I didn't notice it until now.

imcvampire
imcvampire previously approved these changes Apr 21, 2018
@imcvampire imcvampire dismissed their stale review April 21, 2018 07:42

There is a missing review

@pexea12 pexea12 merged commit d220fc6 into pexea12:master Apr 25, 2018
@pexea12
Copy link
Owner

pexea12 commented Apr 25, 2018

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.

3 participants