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

fix grid area, fixes #85 #88

Merged
merged 3 commits into from
Nov 26, 2021
Merged

Conversation

jogibear9988
Copy link
Contributor

Types of Changes

Prerequisites

Please make sure you can check the following two boxes:

  • I have read the CONTRIBUTING document
  • My code follows the code style of this project

Contribution Type

What types of changes does your code introduce? Put an x in all the boxes that apply:

  • Bug fix (non-breaking change which fixes an issue, please reference the issue id)
  • New feature (non-breaking change which adds functionality, make sure to open an associated issue first)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

Description

fix grid area

@jogibear9988
Copy link
Contributor Author

Don't know if this is good like it is...

but who is responsible for minification of the property, so if I write it with the MinifyStyleFormatter, only one value is in the text?

@jogibear9988
Copy link
Contributor Author

@FlorianRappl
don't know if you have another check if a css value is a number, so I used int.parse...

but I think this will not work then:

in chrome I can do this:

$0.style.gridArea='99999999999999999999999999999999999'
and
window.getComputedStyle($0).gridArea
will return:
'1000 / auto / auto / auto'

@jogibear9988
Copy link
Contributor Author

so also according to the spec, should we limit the value?
https://www.w3.org/TR/css-grid-1/#overlarge-grids

@FlorianRappl
Copy link
Contributor

I think the clamping makes sense. We should have this, too.

Regarding number check: There are many micro-parsers that do that. One example is the UnitParser (

public static Unit ParseUnit(this StringSource source)
), which can parse and generate a unit (can also be dimension-less, which is a number).

@jogibear9988
Copy link
Contributor Author

@FlorianRappl

would this be better?

  UnitParser.ParseUnit(new StringSource(tuple.Items[0].CssText));
  (or ParseLength)

or is my int.parse okay?

at least the upper one doesn't work at all...

@jogibear9988
Copy link
Contributor Author

@FlorianRappl any news?

@FlorianRappl
Copy link
Contributor

Ah sorry forgot to reply:

Hm usually you are already in a parser context. Not sure why you do the string to stringsource conversion, as you should already have a string source. If you do all that after you got a value then something may be wrong - as the value may not be a valid one anyway.

In any case ParseLength is suitable if its a length unit - the unit parser is the generic one IIRC.

@jogibear9988
Copy link
Contributor Author

jogibear9988 commented Nov 25, 2021

I don't know where you'd like the chance to be. I tried to do it with minimal impact. If no time at the moment to dig so deep into AngleSharp. But my fix works, and does return correct css ( without it's wrong)

And have you also seen, I've 3 other pull req's with questions.

@FlorianRappl
Copy link
Contributor

And have you also seen, I've 3 other pull req's with questions.

Thanks for bringing this up - I did not see them until now. Looks good!

But my fix works, and does return correct css ( without it's wrong)

That's great - fine with me. All I did was answering your question. Let me know if that works for you.

@jogibear9988
Copy link
Contributor Author

Then this could be merged. It works. It could be improved later if needed

@FlorianRappl
Copy link
Contributor

Then this could be merged. It works. It could be improved later if needed

Alright, then let's do that!

@FlorianRappl FlorianRappl merged commit 706ed93 into AngleSharp:devel Nov 26, 2021
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.

2 participants