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

Update line-height-not-important-78fd32.md #1683

Merged
merged 19 commits into from
Feb 7, 2022

Conversation

tbostic32
Copy link
Collaborator

@tbostic32 tbostic32 commented Aug 12, 2021

Removed extra "1.5" and changed failed example descriptions from using recommend -> required.

Need for Final Call:

This will require 2 week Final Call - Significant changes to the applicability, expectation, background, and some to the test cases.


How to Review And Approve

  • Go to the “Files changed” tab
  • Here you will have the option to leave comments on different lines.
  • Once the review is completed, find the “Review changes” button in the top right, select “Approve” (if you are really confident in the rule) or "Request changes" and click “Submit review”.
  • Make sure to also review the proposed Final Call period. In case of disagreement, the longer period wins.

Removed extra "1.5" and changed failed example descriptions from using recommend -> required.
ajanec01
ajanec01 previously approved these changes Aug 12, 2021
_rules/line-height-not-important-78fd32.md Outdated Show resolved Hide resolved
@tbostic32
Copy link
Collaborator Author

tbostic32 commented Oct 7, 2021

@WilcoFiers

In the second part of your comment here #1686 (comment) you mentioned adding another example of using padding to create enough space to pass. I don't think we can do that without changing the expectation; as it is currently written I think it would fail the rule.

Update from ACT TF meeting
We think this rule needs some more major changes, specifically:

  • Applicability should be for only multiline text
  • Expectation should include scenarios where padding and margin is used to separate lines (satisfy distance requirement)
  • Update test cases to reflect changes to expectation

_rules/line-height-not-important-78fd32.md Outdated Show resolved Hide resolved
_rules/line-height-not-important-78fd32.md Outdated Show resolved Hide resolved
_rules/line-height-not-important-78fd32.md Outdated Show resolved Hide resolved
_rules/line-height-not-important-78fd32.md Outdated Show resolved Hide resolved
_rules/line-height-not-important-78fd32.md Outdated Show resolved Hide resolved
_rules/line-height-not-important-78fd32.md Outdated Show resolved Hide resolved
_rules/line-height-not-important-78fd32.md Outdated Show resolved Hide resolved
_rules/line-height-not-important-78fd32.md Outdated Show resolved Hide resolved
_rules/line-height-not-important-78fd32.md Outdated Show resolved Hide resolved
_rules/line-height-not-important-78fd32.md Show resolved Hide resolved
_rules/line-height-not-important-78fd32.md Outdated Show resolved Hide resolved
_rules/line-height-not-important-78fd32.md Outdated Show resolved Hide resolved
_rules/line-height-not-important-78fd32.md Outdated Show resolved Hide resolved
_rules/line-height-not-important-78fd32.md Show resolved Hide resolved
_rules/line-height-not-important-78fd32.md Show resolved Hide resolved
_rules/line-height-not-important-78fd32.md Show resolved Hide resolved
@tbostic32
Copy link
Collaborator Author

Looks like tests are failing because it doesn't like the word unitless (thinks its a misspelling) and the definition of html element is declared but not used.

@Jym77
Copy link
Collaborator

Jym77 commented Dec 16, 2021

Looks like tests are failing because it doesn't like the word unitless (thinks its a misspelling) and the definition of html element is declared but not used.

The unused definitions can simply be removed from the definitions list at the bottom of the file (lines 307 and 315 it seems).
The spelling exception can be added to this file.

Copy link
Collaborator

@Jym77 Jym77 left a comment

Choose a reason for hiding this comment

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

Wondering whether we should have a viewport specified in the Applicability, but I guess I'd like to see what other think about that.

The rest are polishing. So I'm approving now to let other review and give their opinion on the viewport question 👍

_rules/line-height-not-important-78fd32.md Show resolved Hide resolved
_rules/line-height-not-important-78fd32.md Outdated Show resolved Hide resolved
_rules/line-height-not-important-78fd32.md Outdated Show resolved Hide resolved
@tbostic32 tbostic32 dismissed ajanec01’s stale review January 10, 2022 13:51

Many changes have occured since this approval was granted.

@WilcoFiers WilcoFiers merged commit 0d0565c into act-rules:develop Feb 7, 2022
@WilcoFiers
Copy link
Member

Call for review e-mail here: https://lists.w3.org/Archives/Public/public-act-r/2022Jan/0006.html

We forgot to label it. Apologies.

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.

8 participants