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

Normative: add normative optional styling css override #2215

Merged
merged 1 commit into from
Dec 5, 2020

Conversation

codehag
Copy link
Contributor

@codehag codehag commented Oct 27, 2020

This addresses the problem brought up in #2214 -- where the styles for normative optional changes were forgotten (by me) when the weakrefs merge pr was made.

This likely isn't the best way to do this. This pr is here in case we need a stop gap solution until we clarify how best to add these styles in an accessible way. cc @bterlson, @ljharb, @michaelficarra, @syg and @bakkot.

@michaelficarra
Copy link
Member

Screenshot:

image

@michaelficarra michaelficarra added the editor call to be discussed in the next editor call label Oct 27, 2020
@michaelficarra michaelficarra removed the editor call to be discussed in the next editor call label Dec 2, 2020
@syg
Copy link
Contributor

syg commented Dec 2, 2020

We should be explicit about what the unit of optionality is. @codehag Could you add a sentence explicitly defining the unit of optionality to be everything within a Normative Optional colored box?

@codehag codehag force-pushed the missing-css-normative-optional branch from 53e6738 to 8b9a5fb Compare December 3, 2020 15:58
@codehag
Copy link
Contributor Author

codehag commented Dec 3, 2020

let me know if that is what you had in mind...

Screenshot 2020-12-03 at 16 58 58

Also, were there any thoughts about the point @erights raised about this needing to be visible even without css, or is this not likely to be an issue? One suggestion I can think of is that any section (ie 2.1 here) is followed by "Optional" or something to this effect. Then we can drop the prepend text from the css?

@ljharb
Copy link
Member

ljharb commented Dec 3, 2020

We were thinking that ecmarkup can insert the full “normative optional” text explicitly.

spec.html Outdated Show resolved Hide resolved
@codehag
Copy link
Contributor Author

codehag commented Dec 4, 2020

I applied the changes manually. github seems to have some issues applying comment suggestions atm.

@syg
Copy link
Contributor

syg commented Dec 4, 2020

I applied the changes manually. github seems to have some issues applying comment suggestions atm.

That feature has never worked right for this repo, something to do with permissions like if the suggester has write access or something, it breaks.

I also find that suggestions generate these unreadable emails since the spec source has very long lines.

@michaelficarra
Copy link
Member

@syg @codehag I think it might have something to do with spec.html being very large, in the same way that expanding the context around a diff to spec.html fails.

Copy link
Member

@michaelficarra michaelficarra left a comment

Choose a reason for hiding this comment

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

This WFM for now. When we get ecmarkup generating the "normative optional" text for these sections, we can remove the :before pseudo-element selector.

Copy link
Contributor

@bakkot bakkot left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! I'll upstream the CSS into ecmarkup at some point.

@codehag
Copy link
Contributor Author

codehag commented Dec 4, 2020

I'm not quite used to the work flow: do I merge after 3 approvals, or do you? Or do we need one more?

@michaelficarra
Copy link
Member

@codehag We will handle it.

@michaelficarra michaelficarra added the ready to merge Editors believe this PR needs no further reviews, and is ready to land. label Dec 4, 2020
@ljharb ljharb added normative change Affects behavior required to correctly evaluate some ECMAScript source text spec bug labels Dec 4, 2020
@ljharb ljharb changed the title fixes #2214 - add missing normative optional styling css override Normative: add normative optional styling css override Dec 4, 2020
@ljharb ljharb force-pushed the missing-css-normative-optional branch from b0e42f1 to 31f3c2b Compare December 4, 2020 22:26
@ljharb ljharb merged commit 31f3c2b into tc39:master Dec 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
normative change Affects behavior required to correctly evaluate some ECMAScript source text ready to merge Editors believe this PR needs no further reviews, and is ready to land. spec bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants