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.

Sign up for GitHub

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

Jump to bottom

[show-all-data] Updated js logic to include picklist feature #419

Draft
wants to merge 5 commits into
base: releaseCandidate
Choose a base branch
from
Draft

[show-all-data] Updated js logic to include picklist feature #419

wants to merge 5 commits into from

Conversation

waliasandeep
Copy link
Contributor

Describe your changes

Issue ticket number and link

Checklist before requesting a review

  • I have read and understand the Contributions section
  • Target branch is releaseCandidate and not master
  • I have performed a self-review of my code
  • I ran the unit tests and my PR does not break any tests
  • I documented the changes I've made on the CHANGES.md and followed actual conventions
  • I added a new section on how-to.md (optional)

@waliasandeep
Updated js logic to include picklist
67f53f5 
Updated css to use input id instead of textarea
@waliasandeep waliasandeep changed the title Updated js logic to include picklist Updated js logic to include picklist feature #405 May 7, 2024
@waliasandeep waliasandeep changed the title Updated js logic to include picklist feature #405 Updated js logic to include picklist feature May 7, 2024
@tprouvot
Copy link
Owner

tprouvot commented May 7, 2024

Hi @waliasandeep
Thanks for this PR !

I understand the need of visualising and selecting a value from the picklist field but I would like to keep the possibility to paste data as we have in the current behaviour.
Also it would be nice to have the slds classes for picklist.
What do you think?

@waliasandeep
Copy link
Contributor Author

Hi @tprouvot , sure I can look into a way that allows users to paste data.
I had initially added slds classes to the picklist but found that as of now the slds stylesheet is not even referenced in the inspect.html page. If this is not because of some design decision, I am more than happy to add it.

@waliasandeep
Copy link
Contributor Author

Hi @tprouvot ,
I have added the ability where user can now type in or paste data for a picklist as well.
Regarding adding slds style, all other fields would have to be updated as well so that style parameters like width, height font-style etc. are uniform throughout. So we might have to add slds classes to other components as well.

@tprouvot
Copy link
Owner

This is great thank you !
Now can we keep the current behaviour with up and down keys to iterate throught picklist values ?

@waliasandeep
Copy link
Contributor Author

Sure @tprouvot . Have added back that behaviour now.

@tprouvot
Copy link
Owner

Great thank you @waliasandeep !
During my tests I came across the multipicklist use case.

Is it something you want to address in this PR ?
If not I can merge it since it is still available with the edit text function

@waliasandeep
Copy link
Contributor Author

Hi @tprouvot ,
Thanks for reviewing the change! I can probably address it in a different PR as the UX for multi select would be completely different.

@tprouvot tprouvot changed the title Updated js logic to include picklist feature [data-export] Updated js logic to include picklist feature Jun 5, 2024
@tprouvot tprouvot changed the title [data-export] Updated js logic to include picklist feature [show-all-data] Updated js logic to include picklist feature Jun 5, 2024
tprouvot
tprouvot reviewed Jun 5, 2024
View reviewed changes
Copy link
Owner

@tprouvot tprouvot left a comment

Choose a reason for hiding this comment

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

Thanks @waliasandeep for the updates, here some comments about little details.
During my test I've seen that the input button under the picklist display the value which can be weird for the user

image

addon/inspect.js
@@ -1451,7 +1459,7 @@ class FieldValueCell extends React.Component {
let {row} = this.props;
if (row.tryEdit()) {
let td = e.currentTarget;
row.rowList.model.didUpdate(() => td.querySelector("textarea").focus());
row.rowList.model.didUpdate(() => td.querySelector("#input-field").focus());
Copy link
Owner

Choose a reason for hiding this comment

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

Instead of using querySelector, could you use refs ?

Suggested change
row.rowList.model.didUpdate(() => td.querySelector("#input-field").focus());
row.rowList.model.didUpdate(() => this.refs.inputField.focus());

You can find other examples in the code

addon/inspect.js
h("option", { value: option.value, key: option.value }, option.label)
)
),
h("input", { type: "text", placeholder: "add/select a value", value: row.dataEditValue, onChange: this.onDataEditValueInput, onKeyDown: this.onKeyDown, id: "input-field", style: { width: 'calc(100% - 40px)'}}, null),
Copy link
Owner

Choose a reason for hiding this comment

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

I would prefer to avoid using style property directly

@tprouvot tprouvot marked this pull request as draft August 21, 2024 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tprouvot tprouvot tprouvot left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@waliasandeep @tprouvot