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 EmbedLiveSample macro heights, part 1 #16998

Closed
wants to merge 1 commit into from

Conversation

OnkarRuikar
Copy link
Contributor

Some of the EmbedLiveSample macro outputs got vertical scrollbars after the PR mdn/yari#5337. Due to the shortened heights, the outputs are obscured.

The PR updates live samples that have vertical scrollbars due to their height falling short by small margin.
In order to include lower resolutions, the heights have been updated considering 600px wide iframe.

Note: Only the live samples with scrollbars have been targeted.

@OnkarRuikar OnkarRuikar requested a review from a team as a code owner June 5, 2022 11:03
@OnkarRuikar OnkarRuikar requested review from wbamberg and removed request for a team June 5, 2022 11:03
@github-actions github-actions bot added the Content:WebAPI Web API docs label Jun 5, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jun 5, 2022

Preview URLs

Flaws

None! 🎉

External URLs

URL: /en-US/docs/Web/API/Canvas_API/Tutorial/Basic_usage
Title: Basic usage of canvas
on GitHub

No new external URLs


URL: /en-US/docs/Web/API/Canvas_API/Tutorial/Drawing_shapes
Title: Drawing shapes with canvas
on GitHub

No new external URLs


URL: /en-US/docs/Web/API/Canvas_API/Tutorial/Applying_styles_and_colors
Title: Applying styles and colors
on GitHub

No new external URLs


URL: /en-US/docs/Web/API/Canvas_API/Tutorial/Drawing_text
Title: Drawing text
on GitHub

No new external URLs


URL: /en-US/docs/Web/API/Canvas_API/Tutorial/Using_images
Title: Using images
on GitHub

No new external URLs


URL: /en-US/docs/Web/API/BatteryManager/chargingtimechange_event
Title: BatteryManager: chargingtimechange event
on GitHub

No new external URLs


URL: /en-US/docs/Web/API/BatteryManager/levelchange_event
Title: BatteryManager: levelchange event
on GitHub

No new external URLs


URL: /en-US/docs/Web/API/BatteryManager/chargingchange_event
Title: BatteryManager: chargingchange event
on GitHub

No new external URLs


URL: /en-US/docs/Web/API/BatteryManager/dischargingtimechange_event
Title: BatteryManager: dischargingtimechange event
on GitHub

No new external URLs


URL: /en-US/docs/Web/API/AbstractRange
Title: AbstractRange
on GitHub

No new external URLs

@wbamberg
Copy link
Collaborator

wbamberg commented Jun 5, 2022

I'm not sure why we are doing this in content rather than fixing it in the platform as requested in mdn/yari#6445. How many examples are affected by this, and how many PRs are we asking people to review, by choosing to fix this in content rather than in the platform?

Do these PRs check that the examples don't want to have scrollbars? Some examples do, such as https://developer.mozilla.org/en-US/docs/Web/API/Intersection_Observer_API/Timing_element_visibility .

@OnkarRuikar
Copy link
Contributor Author

There are only 603 pages to fix. Rest of the pages do not have the scrollbar in their live samples. This is nothing compared to #16255.

The PR doesn't simply search and replace the heights. The script actually renders the live samples in a browser, detects the presence of the scrollbar, and computes required height to eliminate the scrollbar. Following is the basic algo:

1. Fetch a page raw data.
2. Extract all live samples.
3. For each live sample
    3.1. Render the live sample in an iframe(which has exactly the same CSS as on MDN web docs).
    3.2. If it has a scrollbar
       3.2.1 Calculate required additional height(reqHeight) to eliminate the scroll.
       3.2.2  If reqHeight > 100, return, do nothing. Because this is a big output and it needs the scrollbar.
       3.2.3  Final height(finHeight) = current height + reqHeight.
       3.2.4  If finHeight < 100, then use 100 in the macro. This will make small outputs look consistent.
       3.2.5  Use finHeight in the macro.

Extra 10px have been added to accommodate variations like user defined font/font-sizes etc.
This also takes care of the cases where simply adding 34px doesn't solve the problem.

Here is the script in action:

updatescroll.mov

@teoli2003 to answer your question about auto detecting heights, this is how it could be done. Yari could use a headless browser.


How many examples are affected by this

~600 pages(using the script mentioned above).

Do these PRs check that the examples don't want to have scrollbars?

The point 3.2.2 in above algo covers this.

Using the PR companion the reviewers have to simply open the preview links in new tabs and skim the pages to see if the scrollbars remained.
I believe we would finish this long before a consensus is reached and yari gets this implemented and deployed.

@OnkarRuikar
Copy link
Contributor Author

Fun fact. After running all the live samples this way I found some with bugs. e.g. #17017, #17019, #17020.
:)

@wbamberg
Copy link
Collaborator

@OnkarRuikar I think we can close this, yes?

@OnkarRuikar
Copy link
Contributor Author

@OnkarRuikar I think we can close this, yes?

Though less annoying now but those scrollbars aren't completely gone after the recent yari patch. Also, we didn't have to sacrifice the padding. 😩

I was waiting for #6561 to get in... Anyway as we are not going this route lets close this.


It was fun to implement the script. Let me know if such work is needed in any other areas. Like new titles for the pages for better SEO.

@wbamberg
Copy link
Collaborator

@OnkarRuikar I think we can close this, yes?

Though less annoying now but those scrollbars aren't completely gone after the recent yari patch. Also, we didn't have to sacrifice the padding. 😩

I was waiting for #6561 to get in... Anyway as we are not going this route lets close this.

Ah, right. Still I agree with closing this now.

It was fun to implement the script. Let me know if such work is needed in any other areas. Like new titles for the pages for better SEO.

It seems like you found lots of live sample errors with the script, so it was useful!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:WebAPI Web API docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants