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

merge my fork in #34

Closed
pwelter34 opened this issue Aug 27, 2019 · 7 comments · Fixed by #46
Closed

merge my fork in #34

pwelter34 opened this issue Aug 27, 2019 · 7 comments · Fixed by #46

Comments

@pwelter34
Copy link

Its been suggested that I merge my fork back into this project. However the code has drifted a lot. Do we still want me to merge my source in?

Issues/Differences

  1. I made large changes to the html and css structure. We'd need to pick the best UI.
  2. I removed the need for the separator form input control.
  3. Added multi-select support
  4. Added keyboard selection support
  5. Added support for binding to a diff value then what the search result is

thoughts?

Demo: LoreSoft Blazor Controls
Repo: LoreSoft.Blazor.Controls

thanks,
~ Paul

@vertonghenb
Copy link
Contributor

vertonghenb commented Aug 28, 2019

@pwelter34 Paul, looks great! If I knew I wouldn't have started the last PR. Maybe we can combine the 2? Personally, I don't like the search bar, but you'll probably need that for multi values. The other stuff looks great especially the 2 controls merged into 1. I'm willing to abandon the last PR but then we need to make some design decisions.

What I like

  • Arrow key functionality
  • the overall styling, especially the grey indicator on the selected value(s)
  • The initial list of items
  • 1 control for Form and Standalone (kudos for that one)
  • The Convert Method

What needs to be improved:

  • Escape / Tab functionality
  • Searchbar, can't we have this in the same input control as the selected value(s)?
  • Arrow Keys in big lists so that the scrollbar navigates with it (might be easier to focus the next element, then the scrollbar is handled by the browser)
  • Clear button, I think it's currently not working for multi-select.

Just my 2 cents, @chrissainty what do you think?

@pwelter34
Copy link
Author

@vertonghenb thanks for feedback.

  • I'm neutral in the look and feel. I personal don't like the search covering the current selected value, but most typeahead do it that way so i'm willing to change it.
  • My version supports keyboard selection with up and down arrows as well as enter to select. I can add escape support.
  • I fixed the clear button on multiselect. It was an issue in the demo code, not the control.

If @chrissainty agrees, i can merge in my stuff. I'll refactor to match public property names as much as possible. I think having one good version is best for community.

~ Paul

@vertonghenb
Copy link
Contributor

vertonghenb commented Aug 28, 2019

@pwelter34
Agreed, one good version should be best for the community.
Therefore I believe it's best to merge in your changes with the following improvements:

  • 1 control to search/show the values, I think it's the way to go and makes the design cleaner, you can check out the Sync Fusion Control, which does this.
  • Support Escape, you can check out the escape fix I made using the event system and Interop class.
  • Support Tab, currently when you tab into the search, you proceed to the next form control. I think it would be better to go to the first option in the list, basically adding a tabindex=0 should suffice. However this is subjective so you can do whatever you like with this.

@chrissainty
Copy link
Member

chrissainty commented Aug 28, 2019

Great discussion. I agree very much with getting your changes merged in @pwelter34.

I think @vertonghenb comments above make a lot of sense. I also have a suggestion or two but perhaps it would be best to get the PR raised then we can sort things out on there?

My main concern though is keeping the public API intact.

What do you both think?

@vertonghenb
Copy link
Contributor

@pwelter34 any update on this? You can reach out to me on Gitter if you need something.

@pwelter34
Copy link
Author

sorry, was on vacation most of last week. I'll get a PR open early this week.

@vertonghenb
Copy link
Contributor

vertonghenb commented Sep 10, 2019

@pwelter34, how is the PR coming along? Do you need help with anything?

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 a pull request may close this issue.

3 participants