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

fix: support editing listbox title #1161

Closed
wants to merge 4 commits into from
Closed

Conversation

quanho
Copy link
Collaborator

@quanho quanho commented Mar 17, 2023

Motivation

To fix qlik-oss/sn-list-objects#158 by checking constraints.active. If it is true, it means we are in edit mode so a listbox label can be edited.

Editable title

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

@quanho quanho requested review from DanielS-Qlik, johanlahti, T-Wizard, veinfors and Caele and removed request for DanielS-Qlik and johanlahti March 17, 2023 16:57
@johanlahti
Copy link
Contributor

johanlahti commented Mar 20, 2023

Does it also work with keyboard navigation in general?

Unit tests?

@quanho
Copy link
Collaborator Author

quanho commented Mar 20, 2023

Does it also work with keyboard navigation in general?

Unit tests?

There is no support for keyboard navigation in Edit mode.

@quanho quanho requested a review from johanlahti March 20, 2023 07:53
@Caele
Copy link
Collaborator

Caele commented Mar 20, 2023

Won't this be pretty strange in a mashup?
Say that you want to render without selections etc, then you for some odd reason would be able to edit the title?

Quickest way to get it working would be to pass in an option from the filterpane to enable the behaviour (and not document it to begin with).

@quanho
Copy link
Collaborator Author

quanho commented Mar 20, 2023

Won't this be pretty strange in a mashup? Say that you want to render without selections etc, then you for some odd reason would be able to edit the title?

Quickest way to get it working would be to pass in an option from the filterpane to enable the behaviour (and not document it to begin with).
Can you use constraints.active in mashup?

@@ -237,6 +238,13 @@ function ListBoxInline({ options, layout }) {
containerPadding = layoutOptions.layoutOrder === 'row' ? '2px 4px' : '2px 6px 2px 4px';
}

const TitleComponent = constraints?.active ? (
Copy link
Collaborator

Choose a reason for hiding this comment

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

there need also be a check of permission if the user has can do setProperties on the object

@@ -78,6 +79,7 @@ function ListBoxInline({ options, layout }) {
calculatePagesHeight,
showGray = true,
scrollState = undefined,
isInSenseClientAndAlowEdittingTitle = false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
isInSenseClientAndAlowEdittingTitle = false,
isInSenseClientAndAllowEditingTitle = false,

Comment on lines +243 to +249
isInSenseClientAndAlowEdittingTitle && constraints?.active ? (
<EditableTitle layout={layout} model={model} />
) : (
<Title variant="h6" noWrap ref={titleRef} title={layout.title}>
{layout.title}
</Title>
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

A Tobias asked: Will the filterpane check app permissions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We discussed about this and see we don't need to check since we do not support filter pane in storytelling which is the case we concern.

@quanho quanho closed this Mar 20, 2023
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.

Not possible to click on the listbox title to edit
5 participants