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

I have a number of updates... #717

Closed
9 tasks done
SteveTheTechie opened this issue Jan 5, 2018 · 24 comments
Closed
9 tasks done

I have a number of updates... #717

SteveTheTechie opened this issue Jan 5, 2018 · 24 comments

Comments

@SteveTheTechie
Copy link
Contributor

SteveTheTechie commented Jan 5, 2018

Thank you for approving my pull requests.

I have been using a older version of this widget for a few years that I have made a number of updates to on my own. The widget has had (and continues to have) a lot of potential and value. However, because I made my changes on my version and there have obviously been changes made here, there are quite a few differences at this point.

Some of my changes I made in the older copy were intended to accomplish the following:

  • 1. Make the widget more performant--the original version I started with was very slow in some cases and used jQuery excessively to the point where it was killing speed. In particular, initializing the widget successively in a large table shows just slow it is--the pop-up list generation seems to be the main culprit. A blended "less is more" strategy that uses either more cached jQuery and/or less jQuery (more straight JavaScript) results in a faster widget.

  • 2. Implemented capability to automatically determine best minWidth based on underlying <select> options.

  • 3. Implemented ability to change the text used for the default links in the header or omit specific default links. This is very useful when maxSelected is in effect, since Check All will likely cause the number of selections to exceed the maximum, but Uncheck All still has value then.

  • 4. Implemented "Flip All" header link that toggles the state of each check box.

  • 5. Changed the update routine to correctly reposition the menu (down) for a growing list of selections in the button which causes the button height to grow.

  • 6. Ditched the use of jQuery UI icons, in favor of similar HTML entities. The jQuery UI icons are not scalable, whereas the HTML entities are.

  • 7. Ditched the multiple option. The multiple attribute of the underlying element is used instead in order to enforce the widget behavior to align with the select element's attributes.

  • 8. Implemented capability to set the menu height based on the size attribute of the underlying select element.

  • 9. Implemented a built-in maxSelected behavior and option. If maxSelected is set to a number, the default alert message is displayed. If it is set to a function name, the function is ran and behavior depends on return value. Not really completed yet.

If you are open to it, I would like to incrementally post a number of pull requests to get the two versions closer together. I also have a number of additional enhancements I have been putting off making.

Can you tell me what your maintenance objectives are for this widget, moving forward, so that I am not spending time on updates that are not wanted?

@SteveTheTechie
Copy link
Contributor Author

The comment numbering does not seem to work as I expected here. There are more than 3 items above.

@mlh758
Copy link
Collaborator

mlh758 commented Jan 5, 2018

If I edit your comment I can see the full list. Probably a format keyword somewhere that's messing the whole thing up. Reading through it now.

Edit: I edited your comment just to make the numbering work. That was bothering me.

@mlh758
Copy link
Collaborator

mlh758 commented Jan 5, 2018

Okay, I'll go down the list and we can figure out some priorities. I'd love to take PRs and release another version:

  1. I've been doing performance stuff with the widget as well. 2.0 included a number of performance related changes, mostly by moving to plain JavaScript in areas where I had open issues complaining about performance. If you have other areas that can improve it I'd be happy to take the changes. There's an old pr that used lazy evaluation to improve load times. It's too out of date to merge but it's something I've been wanting to include.

  2. I've seen a lot of issues mentioning width. If you have ways to improve it this would be a fantastic thing to bring in and probably the one I would be interested in seeing first.

  3. I'd be open to this, but would consider it a lower priority to some of your other proposed changes.

  4. Sounds like a relatively straightforward one, I'd be happy to merge it.

  5. Does this one still happen on 2.0.1? I made some changes around the positioning of the widget.

  6. Fantastic.

  7. Probably a more reasonable fix for Changing multiple option clears selected values #712

  8. This sounds useful as well.

When I first took over the widget I mainly had a few fixes in mind for my own use, and then started going through the backlog of issues trying to resolve them. I don't use the widget a lot myself anymore so I haven't been doing as much work on it. I'm still willing to make code changes where it's needed though. I'm happy to keep maintaining it so your proposed changes wouldn't be wasted. People are still downloading it from npm and I get issues opened here and there so it's under active use.

@SteveTheTechie
Copy link
Contributor Author

Ok, let me get back to you then when I have some time to dig into this a bit more.

@SteveTheTechie
Copy link
Contributor Author

Ok, I have created commits to address 3, 4, 6, and 7.

Are you going to merge or do you have comments/changes you want me to make?

@SteveTheTechie
Copy link
Contributor Author

SteveTheTechie commented Jan 11, 2018

For 2 and 8 above, I had implemented the use of an "auto" keyword for the appropriate option instead of a numeric value in my old fork of the widget code. ("size" could also be used for the automatic height determination as an alternative option keyword.) 8 also addresses #569

The keyword is detected and causes code to run to try to determine the optimal dimension (width or height) based on the characteristics of the menu options. e.g. number of them displayed determined by size attribute of underlying native select and height of individual option for the height case. Also, determination of maximum option width for the auto width determination.

@mlh758
Copy link
Collaborator

mlh758 commented Jan 11, 2018

I'll be looking to merge this weekend. I've been busier with my day job this week and haven't had time to review your new PR.

@SteveTheTechie
Copy link
Contributor Author

SteveTheTechie commented Jan 11, 2018

That is fine... was just checking. I do have some motivation to get all this wrapped up and get focussed back on my project that I use this widget in. (I use this widget quite a bit.)

FYI: The new PR was actually supposed to be 2 PRs, 1 containing the updated files for the improved multiple approach and 1 containing the link text/icon improvments + the Flip All toggling functionality.

Unfortunately, when I merged the 2nd set of commits to my version 3 branch GIthub assumed I was just adding more commits to the existing PR here. I am not sure how to split into 2 PRs (my intent)... again, very sorry, I am still learning GitHub.

@mlh758
Copy link
Collaborator

mlh758 commented Jan 11, 2018 via email

@SteveTheTechie
Copy link
Contributor Author

SteveTheTechie commented Jan 14, 2018

  1. Does this one still happen on 2.0.1? I made some changes around the positioning of the widget.

I have now verified that this is still an issue in the current version update code. I have fixed it already in my fork of the old version, so I will just migrate over my fix.

The issue is that the button height can change if sufficient selections are added or removed. The result is that the menu can appear to overlap the button (button height larger case) or it can seem to have a gap from the button.

The fix is to detect that the menu is open and track when the button height has changed. If is has changed, then position() should be called to force the menu to line up in the correct location.

@SteveTheTechie
Copy link
Contributor Author

SteveTheTechie commented Jan 30, 2018

Coming back to this thread... Added check boxes in original post above.

Also:

  • 10. Need an option to enable shutting off text-wrapping. This has an impact on width determinations.
  • 11. Ditch _toggleChecked in multiselect.filter.js
  • 12. Evaluate feasibility of other issues & PRs posted for potential inclusion in Version 3.
  • 13. Evaluate other possible inclusions from a review of Chosen and Select2
  • 14. Review and update tests. Also, included qunit.js is very outdated.
  • 15. Review and update demos. It would be nice if they could be more interactive.

@mlh758
Copy link
Collaborator

mlh758 commented Jan 30, 2018

#231 Should be easy enough to add in.

@SteveTheTechie
Copy link
Contributor Author

SteveTheTechie commented Feb 2, 2018

@SteveTheTechie SteveTheTechie mentioned this issue Feb 6, 2018
7 tasks
@SteveTheTechie
Copy link
Contributor Author

@mlh758 FYI: I am planning to work on PRs for #733 and #737 next.

@LauraGy
Copy link

LauraGy commented Feb 9, 2018

Hello,
Thanks a lot for your great jobs. This widget is great. The main limitation I see currently is the collapse/expand
@SteveTheTechie, do you think you can inlcude collapse/expand feature like #315 instead of #231 (as @mlh758 mentioned) or #465 as it seems well advanced?

Also, when do you plan to have the version 3 done ?

Thanks again !

@SteveTheTechie
Copy link
Contributor Author

@LauraGy I am not on a timeline... All I can say is check back as to where things stand.

@SteveTheTechie
Copy link
Contributor Author

@LauraGy #751 includes collapsable option groups and a number of other new features.

@LauraGy
Copy link

LauraGy commented Feb 26, 2018

Thanks a lot !

@polexo
Copy link

polexo commented Feb 27, 2018

Hello guys, can you add a type def file to DefinitelyTyped, it will help a lot of people who using typescript, including me :)

@SteveTheTechie
Copy link
Contributor Author

@polexo I have no idea how to do that.

@mlh758
Copy link
Collaborator

mlh758 commented Feb 27, 2018

@polexo Could you open that as a separate issue? I've made some use of TypeScript but never written my own typedefs. It'll be something to look into separately.

@mlh758
Copy link
Collaborator

mlh758 commented Mar 9, 2018

Mind if I close this one @SteveTheTechie ?

@SteveTheTechie
Copy link
Contributor Author

@mlh758 I will close it now.

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

No branches or pull requests

4 participants