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: added fw-list-options component and wiring up of fw-popover #252

Merged
merged 12 commits into from
Nov 3, 2021

Conversation

kishore-kumar-E3682
Copy link
Contributor

@kishore-kumar-E3682 kishore-kumar-E3682 commented Oct 29, 2021

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • My commits have standard messages as mentioned in Contributing Guidelines

How Has This Been Tested?

@kishore-kumar-E3682 kishore-kumar-E3682 changed the title Options list feat: added fw-list-options component and wiring up of fw-popover Nov 2, 2021
renamed methods for fw-option-list and moved hardcoded strings into props.
Copy link
Contributor

@parsuram-vijaysankar parsuram-vijaysankar left a comment

Choose a reason for hiding this comment

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

As discussed in our review meeting let's follow up with the following items in a different PR

  • Dynamic search options
  • Accessibility: keyboard, aria-markers


render() {
return (
<ul>
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's change the ul > li to be a pure div based markup as of now!

We should address the markup with the right accessible attributes as part of a follow up PR!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure Parsu, Will add accessible attributes in follow up PR

}
}

renderSelectOption(options: Array<any>) {
Copy link
Contributor

Choose a reason for hiding this comment

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

rename
renderSelectOption > renderSelectOptions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion i have renamed it.

/**
* Placeholder to placed on the search text box.
*/
@Prop() searchText = 'Search...';
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove the "..."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test Search... is based on the DSM spec.

@kishore-kumar-E3682 kishore-kumar-E3682 merged commit 59c041f into pepperjs Nov 3, 2021
@kishore-kumar-E3682 kishore-kumar-E3682 deleted the options-list branch November 3, 2021 05:59
@kishore-kumar-E3682 kishore-kumar-E3682 mentioned this pull request Dec 2, 2021
6 tasks
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.

2 participants