-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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: Fix long dashboards screenshot emails #15954
Conversation
Codecov Report
@@ Coverage Diff @@
## master #15954 +/- ##
==========================================
+ Coverage 76.99% 77.13% +0.13%
==========================================
Files 988 996 +8
Lines 52371 55481 +3110
Branches 6621 7308 +687
==========================================
+ Hits 40324 42793 +2469
- Misses 11824 12454 +630
- Partials 223 234 +11
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
166f4fe
to
779d63c
Compare
…/fix-report-header
779d63c
to
b52b033
Compare
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.
Left a few comments.
superset/utils/webdriver.py
Outdated
HIDE_NAV = (1,) | ||
HIDE_NAV_AND_TITLE = (2,) | ||
REPORT = (3,) |
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.
HIDE_NAV = (1,) | |
HIDE_NAV_AND_TITLE = (2,) | |
REPORT = (3,) | |
HIDE_NAV = 1 | |
HIDE_NAV_AND_TITLE = 2 | |
REPORT = 3 |
Otherwise the type is not int
, but Tuple[int]
.
@@ -182,8 +182,9 @@ const DashboardBuilder: FC<DashboardBuilderProps> = () => { | |||
? dashboardLayout[rootChildId] | |||
: undefined; | |||
const isStandalone = getUrlParam(URL_PARAMS.standalone); |
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 was a bit confusing, the name isStandalone
suggests that this is a boolean. We should rename this to standaloneMode
or something more clear.
superset/utils/webdriver.py
Outdated
@@ -97,6 +104,7 @@ def get_screenshot( | |||
self, url: str, element_name: str, user: "User", | |||
) -> Optional[bytes]: | |||
|
|||
url = f"{url}?standalone={DashboardStandaloneMode.REPORT}" |
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 will break if we change the URL in utils/screenshots.py
to include other parameters, it would be safer to parse the URL and append the new parameter programmatically instead of doing string manipulation.
An easy way to do that is using the requests
library:
url = f"{url}?standalone={DashboardStandaloneMode.REPORT}" | |
from requests.models import PreparedRequest | |
params = {"standalone": DashboardStandaloneMode.REPORT.value} | |
req = PreparedRequest() | |
req.prepare_url(url, params) | |
url = req.url |
Or we can use urllib.parse
, but it's more code: https://stackoverflow.com/a/25580545/807118
* create serialize json function * create new setting for reports * cleanup * address comments * up the attributes
* create serialize json function * create new setting for reports * cleanup * address comments * up the attributes
* create serialize json function * create new setting for reports * cleanup * address comments * up the attributes
* create serialize json function * create new setting for reports * cleanup * address comments * up the attributes
SUMMARY
Before long reports would have header showing in the middle of the picture. Now i've added a specific
standalone
setting for reports, which will hide the report dashboard and tabs onCapture.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before
After
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION