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: add Imperaitive Handle to public API #1272

Merged
merged 3 commits into from
May 24, 2023
Merged

feat: add Imperaitive Handle to public API #1272

merged 3 commits into from
May 24, 2023

Conversation

Caele
Copy link
Collaborator

@Caele Caele commented May 16, 2023

Motivation

Exposes the Imperative handle API in the form of

  • useImperativeHandle component hook
  • getimperativeHandle viz API function

Requirements checklist

  • Api specification
    • Ran yarn spec
      • No changes OR API changes has been formally approved
  • Unit/Component test coverage
  • Correct PR title for the changes (fix, chore, feat)

When build and tests have passed:

  • Add code reviewers, for example @qlik-oss/nebula-core

@Caele Caele marked this pull request as ready for review May 16, 2023 12:55
@Caele Caele changed the title feat: add Imperitive Handle to public API feat: add Imperaitive Handle to public API May 16, 2023
@Caele Caele requested review from a team, veinfors and niekvanstaveren May 16, 2023 12:56
Comment on lines 991 to 992
* @template T
* @param {function():T} factory
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe simplify to

Suggested change
* @template T
* @param {function():T} factory
* @param {function():object} factory

The generic type T don't give any value

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmm, you could have the handle function return whatever you want, shouldn't it be the generic type then? Not sure how a TS setup handles it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

as this is the only place where the type is referenced I don't think it gives any value.
(but I also don't there is any real problem with keeping it as it is)

@veinfors
Copy link
Collaborator

Looks good to me.
A good idea is to test this change with some existing charts where the useImperativeHandle hook is used already, like the sn-bar-chart, sn-pie-chart etc. to make sure we don't break anything for existing extensions.

@Caele
Copy link
Collaborator Author

Caele commented May 23, 2023

Looks good to me. A good idea is to test this change with some existing charts where the useImperativeHandle hook is used already, like the sn-bar-chart, sn-pie-chart etc. to make sure we don't break anything for existing extensions.

There shouldn't be any functional changes, only typings are added and the API exposed to mashups.

@Caele Caele requested a review from T-Wizard May 23, 2023 13:42
@Caele Caele merged commit e275d3a into master May 24, 2023
12 checks passed
@Caele Caele deleted the tsm/imperative branch May 24, 2023 06:28
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.

None yet

3 participants