CSS selector discussion

[afshin]

Should all of the legacy p-* CSS selectors be renamed to lm-*?

• In the interest of clarity (and purity), my intuition is "yes"
• In the interest of lower churn and a smaller amount of of pain imposed on current consumers of these APIs, "no" seems better.

[jasongrout]

I think this should be a change for Lumino 2.0.

[afshin]

So you think it should be a "yes" ... eventually? Do you think there's room for just leaving them as is or do you consider it too much of an inconsistency in nomenclature to keep the p-* classes?

[jasongrout]

I think the community around Lumino is just starting, and it's more valuable in the long term to have lm- prefixes.

So my intuition is "yes, at the earliest opportunity, which is 2.0".

[jasongrout]

In fact, I think if that is our decision, we should add lm- classes right now, and deprecate p- classes even before 2.0.

[afshin]

In fact, I think if that is our decision, we should add lm- classes right now, and deprecate p- classes even before 2.0.

This is a good idea.

[vidartf]

deprecate p- classes even before 2.0.

I'm not sure what you mean here. The way I read semver, you cannot remove anything (whether deprecated or not) in a minor release.

nevermind, my mind read things to me incorrectly

[afshin]

We won't remove it. We'll add a note that the selector is deprecated, but it will continue to work until 2.0 and the lm-* will work as well.

[afshin]

Hm.

I am not sure we can support the legacy deprecation because when we are checking for p-mod-foo in the TS code, do we now need to check for either p-mod-foo and also lm-mod-foo?

Edit: Perhaps we can. I'm still exploring this in: #20

[afshin]

Okay, so the crux of the question is encapsulated in this example CSS:

/* <DEPRECATED> */
.p-TabBar-tab.p-mod-hidden,
/* </DEPRECATED> */
.lm-TabBar-tab.lm-mod-hidden {
  display: none !important;
}

The .p-mod-hidden inside the deprecated class should never naturally arise once we switch to using lm-mod-hidden, however a consumer may have written custom CSS that targets something with a p-mod-hidden class on it. This is a problem that cannot be solved in pure CSS. So the question that arises is: should we add p-mod-hidden in addition to lm-mod-hidden in the TS logic? This doesn't feel correct to me.

Actually, wouldn't every class need to be added in the TS code? This also doesn't seem like a great answer.

[jasongrout]

In other words, are you asking if it should be as follows while it is deprecated?

/* <DEPRECATED> */
.p-TabBar-tab.p-mod-hidden,
.lm-TabBar-tab.p-mod-hidden,
.p-TabBar-tab.lm-mod-hidden,
/* </DEPRECATED> */
.lm-TabBar-tab.lm-mod-hidden {
  display: none !important;
}

[jasongrout]

So the question that arises is: should we add p-mod-hidden in addition to lm-mod-hidden in the TS logic?

Or are you asking if, in Lumino code, we need to add p-mod-hidden as well as lm-mod-hidden?

I think the answer is yes, we need to add it while it is deprecated.

[afshin]

The trouble is that it's not just the mod classes we need to add. We need to add every class. So we'd have to make sure each widget is both lm-Widget and p-Widget.

[jasongrout]

So we'd have to make sure each widget is both lm-Widget and p-Widget.

Correct, of course. What is the problem with that? That we have a combinatorial explosion of class name combinations?

I think it is fine if we officially support in the documentation all lm-, and all p- classes (but deprecated), but not a mix of both.

[jasongrout]

I was imagining we would not be deleting any code adding classes. Just everywhere we added a p- class, we'd add another line adding the corresponding lm- class.

(Too bad lm looks like Im in many fonts...)

[afshin]

Hm, this actually means that we should not need to add the CSS styles like above at all. We only care about clients who target .p-*, but since our TS will always add .lm-* the stylesheets we ship should not need to have both classes, only the TS should.

[afshin]

This is a good time to bring up the data-lm-suppress-shortcuts again. I actually wonder if this should not be a semantic name like data-orientation instead of a namespaced one like data-lm-dragscroll. The latter is namespaced because it's an implementation detail whereas the former has a generic name because it's public API. So I propose data-suppress-shortcuts, which if it exists means block them. Its value would be immaterial.

[jasongrout]

Hm, this actually means that we should not need to add the CSS styles like above at all. We only care about clients who target .p-*, but since our TS will always add .lm-* the stylesheets we ship should not need to have both classes, only the TS should.

Good point. +1.

[jasongrout]

This is a good time to bring up the data-lm-suppress-shortcuts again. I actually wonder if this should not be a semantic name like data-orientation instead of a namespaced one like data-lm-dragscroll. The latter is namespaced because it's an implementation detail whereas the former has a generic name because it's public API. So I propose data-suppress-shortcuts, which if it exists means block them. Its value would be immaterial.

I would say the distinction is kind of the opposite. data-orientation is only applied to elements Lumino controls, so we don't have to namespace it since it is an implementation detail. data-lm-dragscroll is applied to elements we do not control, so we have to namespace it so it doesn't conflict with whatever else the user might put on it. For that reason, I think data-lm-supress-shortcuts is better, since it would be a class the user is applying to elements under their control, not under Lumino's control.

[afshin]

@jasongrout That's a good point about the data attributes. I agree.

[vidartf]

Hm, this actually means that we should not need to add the CSS styles like above at all. We only care about clients who target .p-, but since our TS will always add .lm- the stylesheets we ship should not need to have both classes, only the TS should.

Unless of course someone is adding the p- classes to their nodes to reuse the existing CSS. Then the question becomes if such use is considered part of the semver guarantee.

[afshin]

Unless of course someone is adding the p- classes to their nodes to reuse the existing CSS. Then the question becomes if such use is considered part of the semver guarantee.

You are correct. But I think if we are deprecating these styles and the use you describe is so edge, I would propose not duplicating the styles in this transition phase. Do you agree with that or do you think it's important to copy the class as well?

[vidartf]

and the use you describe is so edge

Well, I know of one project that are relying on them: https://github.com/jupyterlab/jupyterlab/search?q=p-mod-hidden&unscoped_q=p-mod-hidden

[afshin]

one project ... jupyterlab/jupyterlab ...

Never heard of it.

[jasongrout]

This is legacy and could be removed a long time ago:

https://github.com/jupyterlab/jupyterlab/blob/f5d76fd0447e019c84317cc08c4f6e4ce10fda02/packages/application/style/base.css#L6

These uses are in tests, and arguably we should be using phosphor functions, not the css implementation detail: https://github.com/jupyterlab/jupyterlab/blob/8dc587729e5f2b2fa6087e854f0f988c0232802e/tests/test-apputils/src/toolbar.spec.ts#L201 https://github.com/jupyterlab/jupyterlab/blob/8dc587729e5f2b2fa6087e854f0f988c0232802e/tests/test-logconsole/src/widget.spec.ts#L48

and finally, here we are applying phosphor classes to non-phosphor objects. I think that is wrong and should not be supported: https://github.com/jupyterlab/jupyterlab/blob/76449838443b6ad679d3674f73a2932ac3d8f066/packages/apputils/src/toolbar.tsx#L611-L615

[vidartf]

For completeness, these are the "adding classes to DOM" examples I could find in jupyterlab:

https://github.com/jupyterlab/jupyterlab/blob/8d773bf136876315e1ff6b0483c5d15307d3cce7/packages/extensionmanager/src/widget.tsx#L700

https://github.com/jupyterlab/jupyterlab/blob/8d773bf136876315e1ff6b0483c5d15307d3cce7/packages/outputarea/src/widget.ts#L473

https://github.com/jupyterlab/jupyterlab/blob/8d773bf136876315e1ff6b0483c5d15307d3cce7/packages/statusbar/src/defaults/lineCol.tsx#L103

https://github.com/jupyterlab/jupyterlab/blob/8d773bf136876315e1ff6b0483c5d15307d3cce7/packages/ui-components/src/icon/tabbarsvg.ts#L135

Now, we can say that these are wrong and should not be supported. I'm just saying we should make such a decision with open eyes, and that thinking of cases such as these as "unlikely edge cases" would be a false premise to make the decision on. I would also argue that as part of #3 we should add a section "Styles/CSS" where we document (among other things) what is covered by semver, and what is not.

[afshin]

I think one possible interpretation is that all classes that begin with capital letters like the typescript classes (e.g., `.p-Widget`) are public API. But I don't know. This is a hard problem. For the purposes of this issue, I'm okay with punting and duplicating all the classes with both prefixes until we have a more coherent plan.

…

On Sat, 14 Dec 2019 at 14:05 Vidar Tonaas Fauske ***@***.***> wrote: For completeness, these are the "adding classes to DOM" examples I could find in jupyterlab: https://github.com/jupyterlab/jupyterlab/blob/8d773bf136876315e1ff6b0483c5d15307d3cce7/packages/extensionmanager/src/widget.tsx#L700 https://github.com/jupyterlab/jupyterlab/blob/8d773bf136876315e1ff6b0483c5d15307d3cce7/packages/outputarea/src/widget.ts#L473 https://github.com/jupyterlab/jupyterlab/blob/8d773bf136876315e1ff6b0483c5d15307d3cce7/packages/statusbar/src/defaults/lineCol.tsx#L103 https://github.com/jupyterlab/jupyterlab/blob/8d773bf136876315e1ff6b0483c5d15307d3cce7/packages/ui-components/src/icon/tabbarsvg.ts#L135 Now, we can say that these are wrong and should not be supported. I'm just saying we should make such a decision with open eyes, and that thinking of cases such as these as "unlikely edge cases" would be a false premise to make the decision on. I would also argue that as part of #3 <#3> we should add a section "Styles/CSS" where we document (among other things) what is covered by semver, and what is not. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#9?email_source=notifications&email_token=AABG6KII4NQTRZFUEXPLONTQYTR4FA5CNFSM4JVMPVNKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEG4DI7Y#issuecomment-565720191>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABG6KI6N5PTU26METKGTBLQYTR4FANCNFSM4JVMPVNA> .

[jasongrout]

Going through these too:

https://github.com/jupyterlab/jupyterlab/blob/8d773bf136876315e1ff6b0483c5d15307d3cce7/packages/extensionmanager/src/widget.tsx#L700

We should be using jp-mod-focused here, since it is jlab's component, not phosphor's component.

https://github.com/jupyterlab/jupyterlab/blob/8d773bf136876315e1ff6b0483c5d15307d3cce7/packages/outputarea/src/widget.ts#L473

This is about removing classes, not adding classes. It looks like we just set the class to what we think is the original class. Kind of a kludge, but I think we're accepting there liability for updating that to lm-

https://github.com/jupyterlab/jupyterlab/blob/8d773bf136876315e1ff6b0483c5d15307d3cce7/packages/statusbar/src/defaults/lineCol.tsx#L103

This is not a phosphor class, so it wouldn't be impacted by the switch to lm-. Besides, it is our class, so it should be jp- anyway.

https://github.com/jupyterlab/jupyterlab/blob/8d773bf136876315e1ff6b0483c5d15307d3cce7/packages/ui-components/src/icon/tabbarsvg.ts#L135

Here we are overriding a phosphor class, so again, I think it's reasonable to ask us to change the css to lm- on a major update.

[vidartf]

I think it's reasonable to ask us to change the css to lm- on a major update.

I thought we were discussing "shortcuts" in deprecation for a non-major release, i.e. by not duplicating all CSS rules.

[jasongrout]

I thought we were discussing "shortcuts" in deprecation for a non-major release, i.e. by not duplicating all CSS rules.

I thought we were discussing duplicating all CSS rules for deprecation now (minor release), and eventually changing over in a major release.

My point is that I think, after looking at how JLab is using (or abusing) phoshpor/lumino css classes, there is still not a critical blocker for eventually changing the css to be prefixed lm-.