Refactored Select internaly for Arrays to prepare for other changes #244

wuda-io · 2022-01-11T10:23:46Z

I improved the internal structure of the component, because I saw that there where no Array used for the Options.
So there are no visible changes for the users, but the code is working now internally better and faster.
This is the preperation of dynamically loading Options without reinitializing the Component. But I also thougt
about making the Select similiar to the Autocomplete and give a "data" Object which can then be rendered.

Future Plan:
This should also be done in the Autocomplete. In the Autocomplete it will be a breaking change.
I want to make a combination of the Autocomplete and the Multi-Select with dynamic Loading Data
because I need it for my own Project, but I saw that it was already multiple times requested by other users.

For Reference see issues here:
#219
#67
#115
#171
#124

Types of changes

Bug fix (non-breaking change which fixes an issue).
New feature (non-breaking change which adds functionality).
Breaking change (fix or feature that would cause existing functionality to change).
I have read the CONTRIBUTING document.
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.

…ead of an object

…at the tests pass

Smankusors · 2022-01-11T14:05:54Z

js/select.js

-      if (this.$el.hasClass('browser-default')) {
-        return;
-      }
-


hmm why did you remove most of the comments there?

Because according to clean code, most of the comments were obsolete and had the same name as the functions, so i removed them to keep the code as clean as possible.

haha Clean Code, as I expected 😅

but additional enters / blank lines are fine IMO, no need to remove them (ironically you also add additional blank lines 😄)

so far reading the code LGTM, but I haven't tried it on one of my apps. I will try it on weekends.

wait.. uh oh... I just checked this and it somehow break my app 😅

Though, it's kinda expected because I call the internal _handleSelectChange directly. Need some time to investigate this 🤕

edit: My app didn't break on 026fe0d, so there's something on this PR...

Btw, the reason why I directly call that because the dropdown on the Select didn't update when I set the value of it (by using old jQuery $("[select element]").val(value))

I think IMO I don't need to call this internal function.. So, can you please take a look why the dropdown text didn't update?

Yes thats correct, I also saw that on my Project. The text is not updated correctly. I will investigate again this week an provide an additional commit which fixes this issue. Thank you for taking some time and test it. Looking forward for improvements. Best greets!

I think we can promote the current changes in the develop branch to v1.1.0 since we only have an alpha release of it.

https://github.com/materializecss/materialize/commits/main?after=026fe0d666e7698a50192a34756fa4a3378d7cdf+69&branch=main

https://github.com/materializecss/materialize/tags

Can you coordinate this @Smankusors?
If you need some input from us, please let us know.

After this we can merge the current PRs and make a v2.0.0-rc release.

@Smankusors please check it in your project now. I tested it on my Project and it looks good now. Looking forward for your response.

oh thanks, sorry if this takes days. Running out of time on weekend 😩

I will test it now (edit: it works! thanks)

Can you coordinate this @Smankusors? If you need some input from us, please let us know.

After this we can merge the current PRs and make a v2.0.0-rc release.

wait, coordinate what? To release v2.0.0-rc?

Afaik most of these commens are useful for autogenerating documentation from the annotations.

wait, coordinate what? To release v2.0.0-rc?

No, we are still at v1.1-alpha. With coordinate I mean "feel free to merge if these changes are fine and make a v1.1 (without alpha) release".

I think we can promote the current changes in the develop branch to v1.1.0 since we only have an alpha release of it.

That was what I meant with "coordinate".
You can make a PR for the v1.1 release so the knowledge for doing releases is shared.

Afaik most of these commens are useful for autogenerating documentation from the annotations.

wait, coordinate what? To release v2.0.0-rc?

No, we are still at v1.1-alpha. With coordinate I mean "feel free to merge if these changes are fine and make a v1.1 (without alpha) release".

Yes but the names and descriptions where the same as the function names, so no advantage here. Also the documentation is handwritten for now so i thought i clean up the code and make it more compact. This can be merged from my point of view. Looking forward for some improvement of this project 👍

…tructure

…tion for tests

Smankusors · 2022-01-24T14:23:44Z

js/select.js

@@ -265,13 +265,21 @@
      else this._selectValue(value);
    }
    _getSelectedOptions() {
-      return Array.prototype.map.call(this.el.selectedOptions, (realOption) => realOption);
+      return Array.prototype.filter.call(this.el.selectedOptions, (realOption) => realOption);


eh wait, how does this work? 😅

https://stackoverflow.com/questions/11821261/how-to-get-all-selected-values-from-select-multiple-multiple

I first had troubles with setting things in the DOM. If you refresh your browser, the selected Values are kept in the cache and are not set in the DOM. So I fond the solution here. And since we drop support for IE it works good enough. If it is filter or map does not really matter it only converts the DataType to an Array.

https://caniuse.com/?search=selectedOptions

I think this could also be changed to Array.from() if you want.
https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Array/from
Have you tested it? When should we merge?

If it is filter or map does not really matter it only converts the DataType to an Array.

I see. That's funny that selectedOptions is not an Array haha 😆

Yes it is some DOM specific array which has to be converted for some odd reason...the same issue is with querySelectorAll Method.

Smankusors

LGTM. Others? @materializecss/members

Smankusors · 2022-02-12T16:35:05Z

I think if nobody objects about this, then lets merge this one

thanks btw @wuda-io 👍

wuda-io and others added 7 commits January 10, 2022 22:28

refactor(select): changed the internal value storage to an array inst…

5655bb1

…ead of an object

fix(select): select is now working again after refactoring

9f62267

fix(select): tested and working in Select-Testbed

0622fb0

build(select): improved Testbed for Select

8db1b99

fix(select): removed double-rendering of Input, done in change-event

698429f

refactor(select): cleaned unnecessary spaces

ac09fcf

fix(select): had to fix getSelectedValues to return values only so th…

f37253e

…at the tests pass

wuda-io requested review from ChildishGiant, DanielRuf and Smankusors January 11, 2022 10:38

wuda-io added the component: select label Jan 11, 2022

Smankusors reviewed Jan 11, 2022

View reviewed changes

wuda-io added 3 commits January 17, 2022 16:41

fix(select): shows text in select-box when creating options dynamically

cb52a6e

fix(select): fixed pre-select bug in multi-select and improved code-s…

1087e38

…tructure

test(select): had to fix and add the default option to Choose your op…

0ff6233

…tion for tests

Smankusors reviewed Jan 24, 2022

View reviewed changes

Smankusors approved these changes Jan 25, 2022

View reviewed changes

Smankusors merged commit 4463268 into materializecss:main Feb 12, 2022

Smankusors added the enhancement New feature or request label Apr 7, 2022

wuda-io deleted the improved_select branch February 20, 2023 13:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored Select internaly for Arrays to prepare for other changes #244

Refactored Select internaly for Arrays to prepare for other changes #244

wuda-io commented Jan 11, 2022

Smankusors Jan 11, 2022

wuda-io Jan 11, 2022

Smankusors Jan 11, 2022 •

edited

Loading

Smankusors Jan 16, 2022 •

edited

Loading

wuda-io Jan 16, 2022

DanielRuf Jan 20, 2022 •

edited

Loading

Smankusors Jan 24, 2022 •

edited

Loading

DanielRuf Jan 27, 2022

DanielRuf Jan 27, 2022

wuda-io Feb 2, 2022

Smankusors Jan 24, 2022

wuda-io Jan 24, 2022

wuda-io Jan 24, 2022

wuda-io Jan 25, 2022

Smankusors Jan 25, 2022

wuda-io Feb 2, 2022

Smankusors left a comment

Smankusors commented Feb 12, 2022

Refactored Select internaly for Arrays to prepare for other changes #244

Refactored Select internaly for Arrays to prepare for other changes #244

Conversation

wuda-io commented Jan 11, 2022

Types of changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Smankusors Jan 11, 2022 • edited Loading

Choose a reason for hiding this comment

Smankusors Jan 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielRuf Jan 20, 2022 • edited Loading

Choose a reason for hiding this comment

Smankusors Jan 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Smankusors left a comment

Choose a reason for hiding this comment

Smankusors commented Feb 12, 2022

Smankusors Jan 11, 2022 •

edited

Loading

Smankusors Jan 16, 2022 •

edited

Loading

DanielRuf Jan 20, 2022 •

edited

Loading

Smankusors Jan 24, 2022 •

edited

Loading