Skip to content

Conversation

@linhnihlgard-work
Copy link
Contributor

Motivation

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

@linhnihlgard-work linhnihlgard-work marked this pull request as draft April 4, 2025 12:33
@Caele Caele marked this pull request as ready for review April 22, 2025 08:06
@Caele Caele requested a review from a team April 22, 2025 08:06
Co-authored-by: Christian Veinfors <[email protected]>
@Caele Caele requested review from T-Wizard and veinfors April 22, 2025 13:28
@Caele Caele requested a review from T-Wizard April 23, 2025 07:59
Copy link
Collaborator

@T-Wizard T-Wizard left a comment

Choose a reason for hiding this comment

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

OK.

I am a bit unsure about setOnBlurHandler
I would like something that is easier to understand and use, but don't have any suggestion on what that would be

@Caele
Copy link
Collaborator

Caele commented Apr 23, 2025

OK.

I am a bit unsure about setOnBlurHandler I would like something that is easier to understand and use, but don't have any suggestion on what that would be

I agree, its messy. The whole thing depends on how we handle keyboard nav, either internally in Nebula or externally. And as we don't run a single Nebula instance in embed we need to keep it external (also as we need the sheet to control the ordering of the charts when tabbing/navigating). There might be a combined way we can simplify things, Lihn is fighting it.
But I think we can go ahead with this one for now.

Copy link
Collaborator

@Caele Caele left a comment

Choose a reason for hiding this comment

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

@linhnguyen-qlik think you can merge when you are satisfied, we'll need to continue tinker with parts of this anyways.

@Caele Caele force-pushed the lgu/toggleFocus branch from 4a4e83b to 983faef Compare April 28, 2025 09:38
@linhnihlgard-work
Copy link
Contributor Author

@Caele The strange thing is that boxplot doesn't have definition.ext but in production it could still toggleViewData. Just comment here so we don't forget to check this out. I am going to merge this for now.

@linhnihlgard-work linhnihlgard-work merged commit 63850fe into main Apr 28, 2025
10 checks passed
@linhnihlgard-work linhnihlgard-work deleted the lgu/toggleFocus branch April 28, 2025 19:23
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.

5 participants