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

Don't force table alignment. #5301

Merged
merged 2 commits into from Sep 19, 2018
Merged

Conversation

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Sep 12, 2018

Fixes #3180.

Styles are now allowed to pass thanks to #5012, but we were still overriding table alignment attributes with our own CSS. This allows marked and others to set their own alignment for tables.

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Sep 13, 2018

Thanks, this has always been a bit confusing. When we did the redesign of tables in the classic notebook (and lab) the intention was to R align as default, but to allow other CSS to override. Does this also change the default to L aligned? Or does this just change the ability to override?

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Sep 13, 2018

Hm, I suppose a test suite may be useful here. The problem with setting text-align here is that it was overriding the markup produced by marked. That uses the old HTML attributes (e.g. align=left) rather than CSS, and it seems that our CSS always wins in that confrontation. I am not sure what happens with other producers of HTML tables (like pandas), but will check. Perhaps it should be up to mimebundle producers to include sufficient styling?

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Sep 13, 2018

Hm, this does appear to affect pandas table rendering. Is there a way to flag a CSS rule as unimportant 😄 ?

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Sep 13, 2018

Kind of annoying. The MDN article for table lists "align" as deprecated: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/table#Attributes .
Nevertheless, the marked devs reverted a CSS-only solution a few months ago...

@ian-r-rose ian-r-rose force-pushed the unset_table_align branch from 8101d15 to 6438d66 Sep 14, 2018
@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Sep 14, 2018

Okay, now this aligns-right for rendered HTML, but not that coming from markdown (thus leaving things like rendered pandas dataframes right-aligned). This is kind of an ugly situation, but I couldn't figure out a way around it, since marked uses deprecated attributes rather than CSS 😕

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 14, 2018

and we're tied to marked since we want absolute compatibility with the classic notebook?

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Sep 14, 2018

It looks to me like they are trying to follow the GFM spec: https://github.github.com/gfm/#example-192
That spec, however, relies on the deprecated align attribute.

I suppose we don't have to use marked. It would make me nervous to swap it out so soon before 1.0, however...

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 14, 2018

I suppose we don't have to use marked. It would make me nervous to swap it out so soon before 1.0, however...

Same here. There are other issues where we debated this, and I thought our conclusion was that (a) the notebook markdown format is very underspecified, and essentially tied to the implementation (b) as the "official" successor to the notebook, we were bending over backwards to have parity with the notebook, including completely compatible with the markdown syntax, which essentially ties us to the library.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 14, 2018

See also #272, where we discuss one other markdown renderer.

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Sep 16, 2018

@gnestor do you have any thoughts about the best way forward here? This works, but is kind of clunky. We could also extend the marked rendere as you did in classic.

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Sep 18, 2018

@blink1073
Copy link
Member

@blink1073 blink1073 commented Sep 19, 2018

Thanks!

@blink1073 blink1073 merged commit 7e67f49 into jupyterlab:master Sep 19, 2018
2 checks passed
@blink1073 blink1073 removed this from the 1.0 milestone Sep 28, 2018
@blink1073 blink1073 added this to the 0.35 milestone Sep 28, 2018
@blink1073 blink1073 mentioned this pull request Sep 28, 2018
31 tasks
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants