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

feat(coverage): add breadcrumbs to deno coverage --html docs #24860

Merged
merged 6 commits into from
Aug 14, 2024

Conversation

lczerniawski
Copy link
Contributor

This PR introduces breadcrumb navigation to the deno coverage --html documentation as suggested in #23670
image

@kt3k kt3k self-requested a review August 3, 2024 12:11
@kt3k
Copy link
Member

kt3k commented Aug 8, 2024

Thank you for doing this!

Because we like to minimize the styling effort, we want to align the document structure as close to Istanbul output as possible.

Istanbul renders the breadcrumbs inside the <h1> tag. Could you move the breadcrumbs inside it (without adding any special css classes. They seem like just <a> tags and texts)

Another minor suggestions:

  • Let's use All files instead of Home
  • Maybe let's not use <a> for the last part of the breadcrumbs as it points to its document itself.

The below is a screenshot of Istanbul output:

Screenshot 2024-08-08 at 17 41 38

@lczerniawski
Copy link
Contributor Author

lczerniawski commented Aug 8, 2024

Thank you for doing this!

Because we like to minimize the styling effort, we want to align the document structure as close to Istanbul output as possible.

Istanbul renders the breadcrumbs inside the <h1> tag. Could you move the breadcrumbs inside it (without adding any special css classes. They seem like just <a> tags and texts)

Another minor suggestions:

  • Let's use All files instead of Home
  • Maybe let's not use <a> for the last part of the breadcrumbs as it points to its document itself.

The below is a screenshot of Istanbul output:

Screenshot 2024-08-08 at 17 41 38

Thanks for the guidance, at the beginning, I thought about adding it to the title but didn't want to mess with existing staff.

I removed custom CSS and rewrote it as you suggested.
image

@bartlomieju bartlomieju added this to the 1.46 milestone Aug 13, 2024
@bartlomieju
Copy link
Member

@kt3k do you think we can land this one now?

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

Sorry for the delay (I was sick a few days).

This looks very good! Thank you for your contribution!

@kt3k kt3k merged commit e2faf50 into denoland:main Aug 14, 2024
17 checks passed
@marvinhagemeister
Copy link
Contributor

I'm late to the party, just wanted to say that this is a great UX improvement!

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