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

Added option to filter tests #212

Merged
merged 8 commits into from
Jan 13, 2022
Merged

Added option to filter tests #212

merged 8 commits into from
Jan 13, 2022

Conversation

SebastianZ
Copy link
Collaborator

This addresses #178 and adds a drop-down list to the sidebar allowing to filter the tests by specification status. The options are "All", "Stable", and the different CSS snapshots. The chosen option is saved in the local storage and read on page load to keep the shown info stable.

For that to work I added a new status field to the specifications which holds the field stability indicating whether a spec. is stable or experimental. Optionally, there are two fields first-snapshot and last-snapshot indicating the first and last snapshot a specification got included as official part of CSS, respectively. last-snapshot indicates that a higher level spec. supersedes a previous one in a later snapshot. I.e. it ensures that only the level listed in the snapshot is shown.

Note that this is still WIP. In its current form I had to drop the counting up animation. Instead, the results are shown right away. Also, I'd like to get some feedback on whether that's generally the right direction I am heading. Are you missing any options? Is the idea of superseding specs ok? Should the default option be changed? Should the options be more fine-grained (maybe even be feature based as discussed earlier)? Should specific features listed in "Safe to Release pre-CR Exceptions" be included even when their specification isn't?

Sebastian

@Zefling
Copy link
Collaborator

Zefling commented Jan 5, 2022

It's really cool is much faster. Anchors work better.

There is something weird:
image

And it would be nicer if we could filter those in "experimental".

@SebastianZ
Copy link
Collaborator Author

SebastianZ commented Jan 5, 2022

It's really cool is much faster. Anchors work better.

Yes, that's a benefit. Hope Lea is fine with that, too. She held onto the animation in the past.

There is something weird:
image

It was reset for every test. I've fixed that now.

And it would be nicer if we could filter those in "experimental".

Good point! I've added a filter for them.

Sebastian

@LeaVerou
Copy link
Owner

LeaVerou commented Jan 5, 2022

I did quite like the animation, and it allows us to have slower tests too, but I'll concede if you both agree with removing it. I'm not one to hold a project back just because of my opinions. :)

Is there a screenshot or something about this change? It would help review it!

@LeaVerou
Copy link
Owner

LeaVerou commented Jan 5, 2022

Note that this is still WIP.

You may want to mark it as draft until it's ready to merge

@SebastianZ
Copy link
Collaborator Author

I also liked the animation somewhat, though it also had some issues. Firstly, the mentioned anchor problem, i.e. you can't get to the spec. by entering a URL with its anchor. Also, with more tests it takes longer and longer until the final values appear. And the displayed time it took to execute the tests is a bit misleading as it takes much longer until they are displayed, so users might not immediately understand what that value means.

So, at least for now, I am for removing it. We may add back an animation solely for the display of the score at some point.

I think with the latest commits the changes aren't WIP anymore. Again, detailed feedback is very welcome.

Here's a screenshot:

Screenshot of patch showing filter drop-down list

Sebastian

@Zefling
Copy link
Collaborator

Zefling commented Jan 6, 2022

The animation was a cool at first, but now it becomes a problem, because there is really a lot of tests. Even to test the addition of new tests, it is little useless, because you have to wait longer and longer. I think it would be beneficial for everyone to remove it.

@SebastianZ
Copy link
Collaborator Author

SebastianZ commented Jan 6, 2022

And it would be nicer if we could filter those in "experimental".

Good point! I've added a filter for them.

Forgot to push that change. Did that now.

Please give it a shot!

Sebastian

csstest.js Outdated Show resolved Hide resolved
@Zefling
Copy link
Collaborator

Zefling commented Jan 6, 2022

Maybe we should move "whatwg" and "svg" in separate group.

@LeaVerou
Copy link
Owner

LeaVerou commented Jan 6, 2022

Now that I saw the screenshot I understand what the PR is about. What a fantastic feature!!! Thank you so much for implementing it!

@SebastianZ SebastianZ removed the request for review from PolariTOON January 6, 2022 18:21
@SebastianZ
Copy link
Collaborator Author

Maybe we should move "whatwg" and "svg" in separate group.

Let's discuss this in a separate issue, please! I'd like to get this feature online first. Afterwards we can still iterate on it and add more filters.

I'll also create new features for the questions I had above, so we can discuss them separately and iterate on what we have now.

Sebastian

@Zefling Zefling self-requested a review January 6, 2022 23:21
Copy link
Collaborator

@PolariTOON PolariTOON left a comment

Choose a reason for hiding this comment

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

Overall, LGTM, i think this is a useful feature.
This allows to see how both the CSS language, and the support for it evolve.

But in practice, i don't think this reduces the noise that much : there's a lot of green in the snapshots coming from the previous better supported versions.
We could still add diffs between snapshots later, like in Kangax's compatibility table for JavaScript.

Just one more question: what makes a spec "stable" or "experimental"?

csstest.js Outdated
}

function runTests(filter = '') {
let specs = [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see you replaced some var keywords by let.
Do we want to embrace ES6-style javascript now? or still being somehow compatible with old browsers?

Copy link
Collaborator

Choose a reason for hiding this comment

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

IE isn't really dead yet. For me, it would be necessary to wait until it's definitely dead (removal of Windows). I let you decide.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

According to caniuse.com, IE has (partial) support for let. The biggest percentage of a non-supporting browser is Opera Mini with ~1%. And the overall support is at ~95%, which seems pretty stable to me.

Personally, I prefer to use ES6+ features as long as there is good browser support for them. If you two believe we should strive for full backwards compatibility, I could change it back to var.
Though I have to admit, I never tried to call the site in a very old browser so far. I assume there are other pieces of code that might be unsupported in older browsers.

Having said that, I am also planning to push #57 forward which requires a good browser coverage. So maybe that requires the JS part to be backwards compatible.

Sebastian

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For me, it would be necessary to wait until it's definitely dead (removal of Windows).

For what it's worth, IE is removed in Windows 11 and finally will be gone in June this year, though it will surely still take many years until nobody uses IE anymore.
According to statcounter IE still has a market share of 0.46%.

Sebastian

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because there's a mixture of var and let after that patch, I decided to roll back to var again for now.

I'll file a follow-up issue on refactoring the code base, instead.

Sebastian

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've filed #216 now to discuss whether and which ES6+ features we want to use. Feel free to comment there!

Sebastian

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this line also be reverted to var?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done. Thank's for the hint!

csstest.js Outdated Show resolved Hide resolved
filter.js Show resolved Hide resolved
@SebastianZ
Copy link
Collaborator Author

@PolariTOON Thank you for the detailed feedback!

But in practice, i don't think this reduces the noise that much : there's a lot of green in the snapshots coming from the previous better supported versions. We could still add diffs between snapshots later, like in Kangax's compatibility table for JavaScript.

That's true. Currently, the snapshot filters include all specifications listed as part of the official CSS definition of the related snapshot. Adding diffs between two snapshots shouldn't be too hard.

Just one more question: what makes a spec "stable" or "experimental"?

Sorry, I added a description to the filters in their title attributes but didn't make that clear in the description!

I defined "stable" as everything that's listed in the latest snapshot plus all the predecessor specifications and those that probably won't change much anymore.
As described in #178, "stable" could also be defined as something that's implemented in at least two different browsers, though that may require a property-based approach.

Sebastian

@SebastianZ
Copy link
Collaborator Author

@PolariTOON I think I've addressed all your points now. For most of them I've created separate issues to discuss them further.

If there's anything left you think should be in this first version of the feature, please tell me! Otherwise, it would be great if you also gave your approval, so it can be merged.

Sebastian

Copy link
Collaborator

@PolariTOON PolariTOON left a comment

Choose a reason for hiding this comment

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

Just two minor nits to address and then i think this is ready to land as a first version.
We can refine the feature and refactor later, indeed.

filter.js Show resolved Hide resolved
csstest.js Outdated
}

function runTests(filter = '') {
let specs = [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this line also be reverted to var?

csstest.js Outdated

function runTests(filter = '') {
let specs = [];
let timeBefore = +new Date;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also this one?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@SebastianZ SebastianZ merged commit c4f2908 into gh-pages Jan 13, 2022
@SebastianZ SebastianZ deleted the filter branch January 27, 2022 21:08
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.

4 participants