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

use new report api in viewer #13821

Open
connorjclark opened this issue Apr 5, 2022 · 6 comments
Open

use new report api in viewer #13821

connorjclark opened this issue Apr 5, 2022 · 6 comments
Assignees
Labels

Comments

@connorjclark
Copy link
Collaborator

currently getting our own warning thrown at us in console

Please adopt the new report API in renderer/api.js.

@adamraine
Copy link
Member

The report api does not support everything in ViewUIFeatures. We will probably need to add the saveGist and refresh callbacks to the base ui features.

@connorjclark
Copy link
Collaborator Author

ViewerUIFeatures extends from the base report features class and we don't really need to modify that.

this issue is just about switching to the new DOM constructor and changing whatever breaks from that

@adamraine
Copy link
Member

this issue is just about switching to the new DOM constructor and changing whatever breaks from that

The warning message is talking about using the API in renderer/api.js, something that is not used in the viewer app and would not change with #15964.

I understand the warning condition looks for this._dom.rootEl, but I think that's because rootEl was introduced for the new API (#13277)

@connorjclark
Copy link
Collaborator Author

connorjclark commented Apr 19, 2024

Just want to get rid of this console warning in the viewer. I don't think it's worth doing more here, but technically there is more to be done.

@adamraine
Copy link
Member

If using the new report API is not a priority and the console warning is just noise at this point, then we should just delete the warning message and track migration to the new API here. #15964 neutralizes the warning but it doesn't actually adopt the new API so I don't think #15964 should close this issue.

@connorjclark
Copy link
Collaborator Author

It's still of relevance to external people using our report renderer. We just don't expect them to need all the things the viewer does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants