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

Mobile friendly tables #875

Merged
merged 6 commits into from
Feb 15, 2017
Merged

Conversation

NejcZdovc
Copy link
Collaborator

Added mobile friendly tables.

Resolves #246

@NejcZdovc NejcZdovc force-pushed the feature/#246-mobile-tables branch 4 times, most recently from 12ffc16 to 9d769cc Compare February 12, 2017 15:47
@NejcZdovc NejcZdovc self-assigned this Feb 12, 2017
@NejcZdovc NejcZdovc changed the title [WIP] Mobile friendly tables Mobile friendly tables Feb 12, 2017
@NejcZdovc
Copy link
Collaborator Author

@skipjack what do you think?

Copy link
Collaborator

@skipjack skipjack left a comment

Choose a reason for hiding this comment

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

@NejcZdovc first off, thanks for your work on this. As I just mentioned in #246, this is a tough problem and, even with all those proposals on CSS Tricks, none jump out as the perfect solution.

That said, I think what you've done here is definitely better than what we have now. The one thing I'm not a huge fan of is adding even more markdown.js utility which increases our dependency on marked. We've been debating on moving away from it since it's not too well supported any more. From what I'm seeing, the additions to markdown.js are basically to add class names, a little extra markup, and then re-implement the normal marked tokenizer for eveything aside from tables... is that right or am I missing something else?

@skipjack
Copy link
Collaborator

skipjack commented Feb 13, 2017

@NejcZdovc my inclination is to tie in some of the styling changes from #440, then merge this and come back to it more in-depth later on if we ever get to moving away from marked.

I think the cleanest way to implement all these customizations in a react environment is to have something parse the markdown and pass that data to react components for any kind of special behavior. The only projects I have come across that seem to offer this functionality are:

My only worry would be trying to pick something that will continue to be maintained.

cc @bebraw @bdougie - interested in both of your thoughts here as well. I know we've discussed this a bit on #178.

@skipjack
Copy link
Collaborator

@NejcZdovc we should try to delineate the the two "rows" better here, they can blend together a bit.

image

In your screenshot in #246 you had removed the horizontal borders within each row:

image

which would be a good start although I'm not sure if this is tough to implement or not.

@NejcZdovc
Copy link
Collaborator Author

@skipjack yes you are correct. I only changed table parser, everything else was copied from an original library. I didn't find a way to override only table renderer so I needed to override tok function.

Regarding design, in this stage I only added basic styling, so that if you like the approach we can go one step further. So let's first check if we all agree to like this change and then invest some more time into CSS changes. Personally I like this approach, because it's a lot more readable on mobile and like you said it's not a perfect solution, but I am not aware of a perfect one 😄

@NejcZdovc
Copy link
Collaborator Author

NejcZdovc commented Feb 14, 2017

@skipjack I removed borders for mobile now

@bdougie
Copy link
Contributor

bdougie commented Feb 14, 2017

@skipjack, having these customizations and overrides would be interesting in a React Component. I like the idea of generally making hi-jacks or overrides more declarative than implied, which is what the markdown.js does a lot of. TBH, it took me a bit of time just to get up to speed with the file, but that might just be a limit of my experience.

I also have reservations to adding more options to the markdown.js but only from the experience trying to swap/update marked to a moreup to date and maintained remarkable or showdown.

@bebraw
Copy link
Contributor

bebraw commented Feb 14, 2017

@bdougie Yup, agreed. Ideally we should be able to push these kind of bits to React side. I would love to replace marked with something actively maintained btw.

That said, it's fine by me if we go with a solution that makes sense from user point of view even if the solution is not technically perfect. So keep up good work. 👍

@skipjack
Copy link
Collaborator

@NejcZdovc looks much better with the removed borders on mobile... one last question:

image

It seems more than just the table contents is wrapped in table-wrap class, is this intentional?

Aside from that I think this is good to go, as you mentioned I'll probably be submitting another PR shortly after we get this one merged to do a bit more styling (somewhat based off of #440).

@NejcZdovc
Copy link
Collaborator Author

NejcZdovc commented Feb 15, 2017

@skipjack No it was not intentionally, I forgot to close table-wrap. It's fixed now.

Copy link
Collaborator

@skipjack skipjack left a comment

Choose a reason for hiding this comment

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

@NejcZdovc awesome work, I'll follow up with another PR soon.

@skipjack skipjack merged commit 60e4a98 into webpack:master Feb 15, 2017
skipjack added a commit that referenced this pull request Apr 2, 2017
Fix, re-sort, and re-format this table. The `<td>`s stopped working
after the changes in #875 (which is perfectly fine as we can use this
simpler implementation).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants