-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
report(report-ui-features): Disable auto expansion of print option via report ui #3578
report(report-ui-features): Disable auto expansion of print option via report ui #3578
Conversation
… Print Expanded ISSUES CLOSED: GoogleChrome#2782
… system print via shortcut ISSUES CLOSED: GoogleChrome#2782
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@karanjthakkar looks like this is still in progress correct? right now Print and Print Expanded will do the same thing whereas we want Print Expanded to preserve existing functionality and Print to have the version without expansion
i.e. a new block here
lighthouse/lighthouse-core/report/v2/renderer/report-ui-features.js
Lines 180 to 184 in 04b685f
case 'print': | |
this.expandAllDetails(); | |
this.closeExportDropdown(); | |
self.print(); | |
break; |
@patrickhulce From what I understand, the code you outlined is the one that runs when you click on the dropdown in the top right corner. For this we want to preserve the expanded functionality so I've just modified the text for the dropdown option. There will no longer be a print option in the dropdown. The user can do a normal print (which does not auto expand) using Ctrl + P or the Print option in File menu. |
Oh oops I misread/didn't notice you were removing the print option entirely from the dropdown! That's fine with me, but I'll defer to report owners if we should have both "Print" and "Print Expanded" in the dropdown, in that case, my comment becomes relevant again :) to @paulirish and @vinamratasingal |
To make sure I understand what's going on: we are changing the default print option to "print expanded" and then also ensuring that when a user does the system print, that they only print the summary view (i.e. 2 page report) and not the 28 page report, yes? If the above is true, I would love to see in the share menu both a "print summary view" (or "print current view") and "print expanded view." What additional information gets printed out in the expanded view @patrickhulce ? |
Expanded view just expands all the disclosure triangle sections in the report for you which is what balloons the size. Sounds like there's desire to have both "Print" (same as triggering system print) and "Print Expanded" (the current logic for the print dropdown with just the text change) options in the dropdown, does that make sense to you too @karanjthakkar? |
If I understand correctly Print, Print Summary and Print Expanded are three different options:
In the old implementation, |
That's exactly what I'm thinking too @karanjthakkar, let's have pt 1 and pt 3 as options in the dropdown and punt on pt 2 for now 👍 |
@patrickhulce done! |
Bump! |
Hmm so I'm confused. I thought we had agreed to have in the menu two options: print summary and print expanded. The print option I referreed to just changing the system print (so if a user goes to file --> print) it prints the summary view if the user hasn't expanded any arrows. Is that accurate? So in the report itself, two new options should be present, print expanded and print summary. It would be helpful Karan if you could attach a screenshot of the new report UI changes. |
wait @vinamratasingal you were planning on having 3 distinct print options in the dropdown? Did you want all the options @karanjthakkar described? I took
To mean that you were onboard with having a "Print" which prints the current view as is and a "Print Expanded" which explodes to the full report (current functionality) |
Er let me clarify. I would like 2 options in the UI: Print Summary and Print Expanded. That's it. With Print, it's just changing what the system print should do. System print should map to printing the current view of the page (which I believe it's already doing, so no need to change). |
Yup, this looks great, thanks Karan! Not sure how to give you the approval but LGTM from my end. Please wait for Paul/Patrick to LGTM before merging, thanks :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for taking care of this @karanjthakkar!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This got approval, so landing this :)
Fixes #2782