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

Code styling and workaround for #27 #28

Merged
merged 2 commits into from
Oct 27, 2017

Conversation

pnicolli
Copy link
Contributor

This PR has the first workaround for the demo that I suggested in #27.
It also brings in some minor adjustments to code style.

@nzambello nzambello changed the title Code styling and workaround for #27 Code styling and workaround for (#27) Oct 27, 2017
@nzambello nzambello changed the title Code styling and workaround for (#27) Code styling and workaround for #27 Oct 27, 2017
Copy link
Owner

@nzambello nzambello left a comment

Choose a reason for hiding this comment

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

Ok, I leaved some comments for unsafe parts, but I agree with your changes in general and OK for code style fixes.

}

function oneRow() {
function ellipsize(rows) {
Copy link
Owner

Choose a reason for hiding this comment

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

If you're doing this, isn't it better to have only a function and calling it with the correct parameter in the click handler?
It'd be cleaner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean having one single click handler instead of five? I thought about that, but I actually wanted to keep the code as easy as possible to read. I can do that anyway, if you prefer.

Copy link
Owner

Choose a reason for hiding this comment

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

Ok, in this way the code is simplier to change if we'll want to add/edit features in the demo.

var didEllipsize = false;

function responsiveListener() {
didEllipsize && window.location.reload();
Copy link
Owner

Choose a reason for hiding this comment

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

It's probably the only way (I'm referring to discussion about this in #27), but it's - you know - a bit violent for the user.
I'd add it only if the user has already called ellipsis with the responsive option flagged, because in the other case he will select responsive and the page will reload leaving responsive unflagged as default.

Do you understand what I mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Huh, yeah, my mistake... I actually did that in the first place, but I was experimenting a more difficult approach and had to clean up. I think I removed that by mistake, gonna re-add it right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this and added the commit

Copy link
Owner

Choose a reason for hiding this comment

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

Exactly, it's ok now

}

function oneRow() {
function ellipsize(rows) {
Copy link
Owner

Choose a reason for hiding this comment

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

Ok, in this way the code is simplier to change if we'll want to add/edit features in the demo.

var didEllipsize = false;

function responsiveListener() {
didEllipsize && window.location.reload();
Copy link
Owner

Choose a reason for hiding this comment

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

Exactly, it's ok now

@nzambello nzambello merged commit bd0441f into nzambello:master Oct 27, 2017
@pnicolli pnicolli deleted the pnicolli_code_style branch October 27, 2017 21:58
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.

2 participants