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

Wrap lists + remove nested divs #78

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

bananaoomarang
Copy link
Contributor

Not sure on how we might want to remove nested divs. Was just messing around with this and it seems to work.

I also tried 'bubbling' them up the tree until they were no longer 'nested', but that felt worse than just unwrapping them.

@bananaoomarang
Copy link
Contributor Author

Maybe something to come back to after undo is merged

@DZGoldman
Copy link
Collaborator

Can we (or I) add "strip cite from links" to this, and maybe prioritize this? That bug in particular is pretty urgent

@bananaoomarang
Copy link
Contributor Author

Not really sure on the stability of these, nested divs in particular was pretty fiddly. Mainly did it to test out the feasibility of extending this code with new rules. Maybe just open a new PR that strips citations and see if we can merge that?

@DZGoldman
Copy link
Collaborator

sounds good

@bananaoomarang bananaoomarang changed the title [Demo] wrap lists + remove nested divs Wrap lists + remove nested divs Nov 17, 2017
source/Node.js Outdated
return true
}

return isParentedBy(node.parentNode, parents)
Copy link
Contributor

Choose a reason for hiding this comment

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

Naming OCD guy here - the moment you start going up the tree, that's an ancestor rather than a parent. I know jquery has taken liberties with using parent, but it's really clear in commonplace English... hasAncestor is a decently clearer name for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Jeez

@bananaoomarang
Copy link
Contributor Author

List stuff seems to work pretty well.

Div stuff definitely works but we possibly want to scope it better to certain situations/do something nicer there. The alternative approach would be to separate them out rather than condense them into their parent. In my testing they both look kind of weird from a user standpoint.

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