Repository navigation
Conversation
onizet
left a comment
onizet
left a comment
There was a problem hiding this comment.
I was using w3schools.com and they never talk about the type attribute on the UL tag. This is a nice addition to make this library more complete. Thank you for all your support recently.
|
|
||
| var paragraphs = mainPart.Document!.Body!.Elements<Paragraph>().ToList(); | ||
| Assert.That(paragraphs, Has.Count.EqualTo(4)); | ||
| Assert.That(paragraphs.Select(GetBulletText), Is.EqualTo(new[] { "•", "▪", "•", "▪" })); |
There was a problem hiding this comment.
This would be better to use [TestCase] and returning that BulletText as ExpectedResult.
| return null; | ||
|
|
||
| if (!orderedList) | ||
| return type!.ToLowerInvariant() switch |
There was a problem hiding this comment.
The null forgiving "!" is no longer needed as you assert first string.IsNullOrEmpty
| if (!orderedList) | ||
| return type!.ToLowerInvariant() switch | ||
| { | ||
| "disc" or "circle" or "square" => type.ToLowerInvariant(), |
There was a problem hiding this comment.
This is a 2nd call ToLowerInvariant() which could be avoided. Just returns "disc" => disc, "circle" => "circle" etc
To avoid that if branch, you can use this pattern matching:
return (orderedList, type) switch
{
(false, var t) when t.Equals("disc", StringComparison.OrdinalIgnoreCase)
=> "disc",
(false, var t) when t.Equals("circle", StringComparison.OrdinalIgnoreCase)
=> "circle",
(false, var t) when t.Equals("square", StringComparison.OrdinalIgnoreCase)
=> "square",
(true, "1") => "decimal",
(true, "a") => "lower-alpha",
(true, "A") => "upper-alpha",
(true, "i") => "lower-roman",
(true, "I") => "upper-roman",
(true, "lower-greek" or "upper-greek") => type,
_ => null
};| "A" => "upper-alpha", | ||
| "i" => "lower-roman", | ||
| "I" => "upper-roman", | ||
| "lower-greek" or "upper-greek" => type, |
There was a problem hiding this comment.
This line hits my eyes because it enforces casing. But I was already reviewing the MDN documentation and those are not part of the W3C standard.
Interesting part is using Git blame, I see that it was you who submitted that code change. Do you use that or was that included by mistake?
There was a problem hiding this comment.
yes, I invluded the greek list marker styles (lower-greek and upper-greek) for a bug which I submitted long back, here is the commit 524efdc#diff-14522a172736ece97f58356f21b2c45c09eddcca8442774351b948d9aea10d05
There was a problem hiding this comment.
Yes, agree I saw your commit. But your previous PR brought support of list-style-type: lower-greek which is a W3C standard. type: lower-greek is non-standard.
As you were the original author of that line change, I'm checking if your intent was to use list-style-type AND type ? If not, you can remove the greek version of type.
There was a problem hiding this comment.
if your intent was to use list-style-type AND type
yes, now that we have merged both attributes (type and list-style-type) resolution in the ListTypeToListStyleType() for both ul and ol elements. we cant differentiate that the type arg is from either of the attribute value
So for ordered list since there is no support for greek markers in legacy "type" attribute, it would always resolved by "list-style-type" inline style attribute
I hope this is what you were looking for as response
|
Hello @onizet yes, our usecase involves sanitized html from word as well, and this html produces such legacy attrbutes dependent html tags. |
|
Hello @onizet new changes pushed, please review Thanks |
| [TestCase("disc", "circle", ExpectedResult = "•,\u25CB,•,\u25CB")] | ||
| public async Task<string> NestedUnorderedList_TypeAttribute_UsesOwnBullet(string listType, string nestedListType) | ||
| { | ||
| await converter.ParseBody($@" |
There was a problem hiding this comment.
Simplify the HTML to the minimum. The goal is not to test consecutive list. So please remove the 2nd <ul>
The [Test] is no longer applicable (as you now accept params) so convert it to /// <summary>
and rename Ingredients to List Item1 and so on.
Thanks!
| (true, "A") => "upper-alpha", | ||
| (true, "i") => "lower-roman", | ||
| (true, "I") => "upper-roman", | ||
| (true, "lower-greek" or "upper-greek") => type, |
There was a problem hiding this comment.
this is case-sensitive which is not defensive. So either applies the same logic as Equals(OrdinalIgnoreCase), either remove lower/upper-greek which are non-standard.
There was a problem hiding this comment.
removed the "lower-greek" and "upper_greek" case
|
comments resolved |
2 participants
Problem Statement
Only ordered lists have the capability to utilize the legacy tyoe attribute
Resolution
Both ordered and unordered list mut utilize their legacy type attribute in absence of inline stle list-style-type attribute Both list tyle types are resolved in the method
ListTypeToListStyleType()