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 :target to Page Inspector pseudo-class toggles #2020

Merged
merged 3 commits into from Feb 22, 2021

Conversation

SebastianZ
Copy link
Contributor

Updated pseudo-class toggle options of Page Inspector to include the :target pseudo-class. And also added it to the release notes for Firefox 87. See bug 1689899.

This change is all mine, from filing the bug over creating the patch plus test to documenting it. 😄 And it will be my first (and surely not last) contribution since the contents moved to GitHub. 🥳

Sebastian

@SebastianZ SebastianZ requested a review from a team as a code owner February 2, 2021 18:56
@SebastianZ SebastianZ requested review from a team and chrisdavidmills and removed request for a team February 2, 2021 18:56
@hamishwillee
Copy link
Collaborator

@SebastianZ Fantastic! I'm working on Friday and will look at this then (assuming it is not reviewed by someone else first). FYI this is erroring because the images you added could be more compressed - see https://github.com/mdn/content/pull/2020/checks?check_run_id=1817070267#step:9:19

As it says, you need to run yarn filecheck "files/en-us/tools/page_inspector/how_to/examine_and_edit_css/show_pseudo_classes_hover.png" --save-compression for each of the files/.

This does not currently work (for me) on windows, but it does on Ubuntu.

@chrisdavidmills
Copy link
Contributor

Hah, lovely to see you here @SebastianZ !

I am going to leave you in @hamishwillee 's capable hands for this one, but please shout if you folks need any assistance (e.g. I'm on macOS, so could apply the image compression to the build if needed).

@chrisdavidmills chrisdavidmills requested review from hamishwillee and removed request for chrisdavidmills February 3, 2021 11:14
@SebastianZ
Copy link
Contributor Author

I am also on Windows and I didn't figure out yet how to get Yarn to run there. I'll see if I can get it to work somehow.

It would be great if there were more information about this than

Note, this is how you do it on terminals with bash (macOS and Linux). You have to do it differently on Windows.

Just as a side note. 😄

Sebastian

@SebastianZ
Copy link
Contributor Author

Ok, got Yarn running by installing it via npm. As @hamishwillee already pointed out, the compression doesn't work though. I get an error that the command cannot be processed syntatically and it results in an exit code 255. 😞 I'll see what I can do.

Sebastian

@hamishwillee
Copy link
Collaborator

@SebastianSimon Hot of the press, there is a workaround for compression on windows. On the file in the content root named package.json change the line:

"filecheck": "env-cmd --silent cross-env CONTENT_ROOT=files node node_modules/@mdn/yari/filecheck/cli.js --cwd=$(pwd)",

TO

"filecheck": "env-cmd --silent cross-env CONTENT_ROOT=files node node_modules/@mdn/yari/filecheck/cli.js --cwd=.",

(ie --cwd=.)

I'm back on this tomorrow.

@SebastianZ
Copy link
Contributor Author

That was it! Thank you very much, @hamishwillee! Especially for helping while not at work!

Sebastian

PS: You mentioned the wrong Sebastian in your comment. 😄

@SebastianSimon
Copy link
Contributor

PS: You mentioned the wrong Sebastian in your comment. 😄

Ah, that’s why I’m randomly receiving emails; I was so confused. 😄

@hamishwillee
Copy link
Collaborator

Many apologies - who would have thought there could be two Sebastians in the world :-).

@hamishwillee
Copy link
Collaborator

Looks great @SebastianZ .

Please see inline comments. Could you also update the screenshot for the image after the one you updated - https://github.com/mdn/content/pull/2020/files#diff-23e77088e1e57c2827717e5b4491d2cdfca0614474fdfec364ea9012edf29c7cR162 - as this also shows the selector toggles.

@SebastianZ
Copy link
Contributor Author

Looks great @SebastianZ .

Please see inline comments. Could you also update the screenshot for the image after the one you updated - https://github.com/mdn/content/pull/2020/files#diff-23e77088e1e57c2827717e5b4491d2cdfca0614474fdfec364ea9012edf29c7cR162 - as this also shows the selector toggles.

Oh, right! How could I miss that? I've replaced that image now, too.

Sebastian

@hamishwillee hamishwillee changed the title Added :target to Page Inspector pseudo-class toggles [WIP Do not merge] Added :target to Page Inspector pseudo-class toggles Feb 5, 2021
@hamishwillee
Copy link
Collaborator

Great!

@chrisdavidmills This is good, but should not be merged until FF86 is in, and users can reasonably expect to start seeing docs for FF87. I have marked as "WIP - do not merge" in the title. Can you create an FF87 project and add this so we don't miss it?

@chrisdavidmills
Copy link
Contributor

I'd be OK to merge this, as version 87 is available as Firefox Nightly, provided we say it is aailable in 87 onwards...

One thing though — I can't see it in my Nightly. Is it behind a flag or something? I just updated.

@Ryuno-Ki
Copy link
Collaborator

Ryuno-Ki commented Feb 8, 2021

I have marked as "WIP - do not merge" in the title.

Would it be possible to also mark it as a Draft PR? That would effectively block the merge in GitHub.

@hamishwillee
Copy link
Collaborator

@chrisdavidmills

version 87 is available as Firefox Nightly, provided we say it is aailable in 87 onwards...

We discussed that the text to add "from v87 onwards" looks pretty ugly here, and we'd need to add a second PR in a month when FF87 comes out to remove it. So we figured we'd rather just wait/not merge.

@Ryuno-Ki I don't have any rights to revert things to draft, which is why I marked heading as WIP. If you do then please make the change.

@Ryuno-Ki Ryuno-Ki marked this pull request as draft February 9, 2021 07:32
@Ryuno-Ki
Copy link
Collaborator

Ryuno-Ki commented Feb 9, 2021

It's a draft now. Let us know once you feel it's complete enough to get merged.
Current subscriptions by people who commented are still in place.

@chrisdavidmills
Copy link
Contributor

We discussed that the text to add "from v87 onwards" looks pretty ugly here, and we'd need to add a second PR in a month when FF87 comes out to remove it. So we figured we'd rather just wait/not merge.

Ah OK, makes sense. Thanks for the clarification.

@hamishwillee hamishwillee marked this pull request as ready for review February 21, 2021 22:21
@hamishwillee
Copy link
Collaborator

@chrisdavidmills This is now ready to go in as fix for #2508

@hamishwillee hamishwillee changed the title [WIP Do not merge] Added :target to Page Inspector pseudo-class toggles Added :target to Page Inspector pseudo-class toggles Feb 21, 2021
Copy link
Contributor

@chrisdavidmills chrisdavidmills left a comment

Choose a reason for hiding this comment

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

I already looked at this...let's get it merged ;-)

@chrisdavidmills chrisdavidmills merged commit af8db57 into mdn:main Feb 22, 2021
@SebastianZ
Copy link
Contributor Author

I guess I can mark the related bug as dev-doc-complete now, right?

Sebastian

@chrisdavidmills
Copy link
Contributor

I guess I can mark the related bug as dev-doc-complete now, right?

Sebastian

yup! And provide a reference to this issue, so the engineers can see what work happened.

hamishwillee pushed a commit to hamishwillee/content that referenced this pull request Feb 23, 2021
* Added :target to Page Inspector pseudo-class toggles

* Compressed screenshot of pseudo-classes panel

* Replaced hover_indicators image by one showing :target pseudo-class
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants