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 performance tab to the Preflight tool (#2773) #3056

Merged
merged 5 commits into from
Oct 29, 2024

Conversation

mokimo
Copy link
Contributor

@mokimo mokimo commented Oct 16, 2024

Description

A few todo's here and there but the features and flow should work and this should be ready for feedback and a first batch of testing.

Look & Feel

Screenshot 2024-10-17 at 01 08 39

Resolves: MWPW-157715

Test instructions

Use the provided sample URLs, try to destroy the thing, close it, open it, open it on incognito, have pages with LCP, without, use production pages from bacom etc. to test this. I've encountered a lot of issues during development and I'm sure theres still many issues out there.

Test URLs:

@mokimo mokimo requested a review from a team as a code owner October 16, 2024 23:20
Copy link
Contributor

aem-code-sync bot commented Oct 16, 2024

Copy link

codecov bot commented Oct 16, 2024

Codecov Report

Attention: Patch coverage is 78.94737% with 56 lines in your changes missing coverage. Please review.

Project coverage is 96.35%. Comparing base (ec48f3c) to head (d5c22fe).
Report is 53 commits behind head on stage.

Files with missing lines Patch % Lines
libs/blocks/preflight/panels/performance.js 78.86% 56 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            stage    #3056      +/-   ##
==========================================
+ Coverage   96.34%   96.35%   +0.01%     
==========================================
  Files         243      246       +3     
  Lines       55284    56012     +728     
==========================================
+ Hits        53261    53972     +711     
- Misses       2023     2040      +17     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mokimo mokimo force-pushed the preflight-performance-tab branch from b9d5250 to 55d3b95 Compare October 16, 2024 23:32
Copy link
Contributor

This pull request is not passing all required checks. Please see this discussion for information on how to get all checks passing. Inconsistent checks can be manually retried. If a test absolutely can not pass for a good reason, please add a comment with an explanation to the PR.

Copy link
Contributor

@overmyheadandbody overmyheadandbody left a comment

Choose a reason for hiding this comment

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

I like the general direction of this, good job! I do, however, have a few notes 😅

}

export const checkFragments = () => conditionalItemUpdate({
failsWhen: config.lcp.element.closest('.fragment') || config.lcp.element.closest('.section')?.querySelector('[data-path*="fragment"]'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this taking deeper nested fragments into account?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but just in the sense of, it tells you that there is a fragment being used: https://preflight-performance-tab--milo--mokimo.hlx.page/drafts/osahin/preflight-performance/marquee-deep-nested-fragments

... we could also surface nested fragments, e.g. "amount of fragments within fragments, 5" 🤔
Wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

I've heard of 4 levels deep nested fragments, so I think we should put this on the idea list, yes.

// TODO MEP/Personalization
// TODO mobile / tablet?
// TODO create ticket for PSI API
// TODO link to documentation directly within the sections
Copy link
Contributor Author

Choose a reason for hiding this comment

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

todo, we could also check for commerce within the marquee

Copy link
Contributor

@overmyheadandbody overmyheadandbody left a comment

Choose a reason for hiding this comment

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

Thanks for addressing all the feedback!

Copy link
Contributor

@robert-bogos robert-bogos left a comment

Choose a reason for hiding this comment

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

Thank you for addressing the feedback! 🙏🏻

@SilviuLCF SilviuLCF self-requested a review October 29, 2024 11:36
Copy link

@SilviuLCF SilviuLCF left a comment

Choose a reason for hiding this comment

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

Copy link

@SilviuLCF SilviuLCF left a comment

Choose a reason for hiding this comment

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

@mokimo mokimo merged commit fded71e into adobecom:stage Oct 29, 2024
17 of 18 checks passed
@mokimo mokimo mentioned this pull request Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants