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

Add support for array style options #15

Open
tdulcet opened this issue Mar 31, 2021 · 11 comments
Open

Add support for array style options #15

tdulcet opened this issue Mar 31, 2021 · 11 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@tdulcet
Copy link

tdulcet commented Mar 31, 2021

This may be out of scope for this library, but it would be very useful if it supported array style options, where the user can add more than one value to an option or more than one instance of an option group. For example, consider a standard weather app/extension where the user can add multiple locations, each with a ZIP code and country. This would also be needed for any input or select elements where the multiple attribute is set.

This is currently blocking rugk/unicodify#30.

@rugk
Copy link
Member

rugk commented Mar 31, 2021

Well… html=multiple only seems to work for file or email inputs, which is of course not what we'd want.

But yeah, multiple options in a kind of multi-select input would be great. However, the UI is totally missing here.
There is not standard HTML component for that.

Apart from that, I don't consider it out-of-scope, it's just hard to implement technically and I never really needed it and could find other solutions, so it's not in there.
If you have an idea or want/need an implementation, I'd be happy to look at PRs and better maybe discuss how we should approach/handle it, before. 😃

@rugk rugk added enhancement New feature or request help wanted Extra attention is needed labels Mar 31, 2021
@tdulcet
Copy link
Author

tdulcet commented Apr 1, 2021

Apart from that, I don't consider it out-of-scope, it's just hard to implement technically and I never really needed it and could find other solutions, so it's not in there.

Yeah, it could require some significant JS to implement the UI.

If you have an idea or want/need an implementation, I'd be happy to look at PRs and better maybe discuss how we should approach/handle it, before. 😃

I have at least three extension ideas which would need this feature, however I think the four other issues I created are a higher priority, especially the two blocking parts of Unicodify and the Awesome Emoji Picker, although I would definitely be interested to hear how you think we could best implement this feature...

Back to my weather extension example, I was thinking developers could specify their default settings like this to indicate that they want to use array style options for an option group:

const defaultSettings = {
	locations: [
		{
			zip: "20500",
			country: "US"
		}
	]
};

@tdulcet
Copy link
Author

tdulcet commented Dec 1, 2021

Apart from that, I don't consider it out-of-scope, it's just hard to implement technically and I never really needed it and could find other solutions, so it's not in there.

Yeah, it could require some significant JS to implement the UI.

For adding more than one instance of an option group, I implemented a simple UI using a table, which has a column for each option in the option group and a row for each instance of the option group: https://jsfiddle.net/4bafm35n/. There is also a button to convert the table to an array, to demonstrate what would be needed by this library to save the values in the table. This library would then just need run that code anytime the user modified any of the cells.

For the select elements where the mutiple attribute is set, there is a one liner here to convert that to an array: https://stackoverflow.com/a/39363742, which is what would be needed for rugk/unicodify#30.

I'd be happy to look at PRs and better maybe discuss how we should approach/handle it, before. 😃

I would also be happy to discuss how we could best update the library to support this feature...

@rugk
Copy link
Member

rugk commented Dec 1, 2021

What you read sounds all goods far as for what you write. A table sounds reasonable as a HTML markup for that, though I maybe would use that in the code, but rather use classes, so you can use your own markup there (after all, maybe the options also are not supposed to be in one line/could spawn multiple lines).

So maybe it could be designed similar to how the options groups are implemented.

Instead of a string as a thing to copy for each entry/"line"/"row", it should use a HTML . Maybe also a slot for where this stuff, but I am not sure how this WebComponent thing is used.

Finally, it just needs a button with another class that is used as the "add another entry" button.
Maybe also a class for the "remove this entry" button that can be used in the <template> to delete that entry.
And finally maybe another class for "clear all".

@tdulcet
Copy link
Author

tdulcet commented Dec 2, 2021

What you read sounds all goods far as for what you write. A table sounds reasonable as a HTML markup for that, though I maybe would use that in the code, but rather use classes, so you can use your own markup there (after all, maybe the options also are not supposed to be in one line/could spawn multiple lines).

Yes, anyone using your library would need to write their own HTML for the table. That was just to demo a possible UI for this, so that we could make the necessary changes to the library. Specifically, it was using the same weather extension example from my above post.

Instead of a string as a thing to copy for each entry/"line"/"row", it should use a HTML . Maybe also a slot for where this stuff, but I am not sure how this WebComponent thing is used.

Yes, I think the HTML Template element would work great here, as the user could then specify their format for each row directly in the HTML file.

Finally, it just needs a button with another class that is used as the "add another entry" button. Maybe also a class for the "remove this entry" button that can be used in the <template> to delete that entry. And finally maybe another class for "clear all".

I am not sure what you mean by "class". A JS class? A CSS class? I updated my demo to support removing all of the added entries/rows, as well as moving individual rows up and down: https://jsfiddle.net/4bafm35n/1/. I think this is everything most extensions would need. I also used emojis to reduce the amount of text that would need to be localized.

Here is another updated version of the demo using the HTML Template element as you suggested: https://jsfiddle.net/4bafm35n/3/. I believe this is all of your suggested changes. It also now ignores any rows with invalid cells when creating the array, using the method suggested in #14.

@rugk
Copy link
Member

rugk commented Dec 5, 2021

what you mean by "class". A JS class? A CSS class?

The CSS one.

I also used emojis to reduce the amount of text that would need to be localized.

Ah indeed a good idea.

ere is another updated version using the HTML Template element as you suggested: https://jsfiddle.net/4bafm35n/3/. This also now ignores any rows with invalid cells when creating the array, using the method suggested in #14.

Nice one.

@tdulcet
Copy link
Author

tdulcet commented Dec 6, 2021

Similar to #13, I think this could be implemented using the AutomaticSettings.Trigger.addCustomLoadOverride() and AutomaticSettings.Trigger.addCustomSaveOverride() functions, but that would obviously result in a lot of code duplication between extensions which eliminates many of the benefits of this library. Unlike #13, which has a smaller use case of extensions that support multiple accounts, I believe this functionality would be used by many extensions, including Unicodify and at least three of the prototype extensions I shared with you, so it would be great if it were supported by this library.

The CSS one.

If I understand you correctly, you are proposing we add a CSS class to each of the buttons. Note that extensions could have multiple options where they need to have more than one instance of an options group and thus they would need multiple tables. This library would then need to know which buttons corresponded to which table. At minimum, the library would need a reference to the table, template for each row, add entry/row button and remove all entries/rows button.

Anyway, I believe I implemented all your suggested changes in my demo, although I am not sure I understand what you are proposing we update in your library. Note that this issue is requesting support for two similar features:

  • Adding more than one instance of an option group.
  • HTML select elements where the multiple attribute is set.

My demo was only a possible UI for the former, since you said:

the UI is totally missing here.
There is not standard HTML component for that.

My demo just has simple text inputs in each row of the table, but extensions could obviously have any arbitrary form elements.

Edit: Here is another updated version of the demo, which adds a default row/entry to the table and fixes a bug with the buttons: https://jsfiddle.net/2uhb0ryw/

@rugk
Copy link
Member

rugk commented Dec 6, 2021

This library would then need to know which buttons corresponded to which table. At minimum, the library would need a reference to the table, template for each row, add entry button and remove all entries button.

Yes, that's easy though. One can get it as this document.querySelector("#TemplatetHIng > .clear-button").


Yeah as for how exactly to implement it I am also not sure, I'd need to look into the lib too anyway. I did not have a look there since quite some time. 🙃

@tdulcet
Copy link
Author

tdulcet commented Jul 2, 2022

I was finally able to integrate my demo with the AutomaticSettings.Trigger.addCustomLoadOverride() and AutomaticSettings.Trigger.addCustomSaveOverride() functions to create a working options page that supports more than one instance of an option group:
image

It actually has five such options that need more than once instance of an options group for each of the five supported issue trackers. Anyway, I found a few issues with this library that I had to work around:

  • HTML input elements without the name attribute set results in an options group with an undefined key. These lines do not first check that the name is defined before adding it to the options group:
    // update data in group with new values from settings HTML page
    document.querySelectorAll(`[data-optiongroup=${optionGroup}]`).forEach((elCurrentOption) => {
    const [currentOption, currentOptionValue] = getIdAndOptionFromElement(elCurrentOption);
    optionValue[currentOption] = currentOptionValue;
    });
  • This library of course only supports the input and change events, but it would likely need to use a MutationObserver to detect when the user adds/removes rows from the table or moves them up/down. I was only able to work around this by manually creating synthetic change events.
  • The registerSave() callback runs before the addCustomSaveOverride() callback, so it is not able to take advantage of any fixes to the data made in the latter. It would be very helpful if we could directly override the HtmlMod.applyOptionToElement() and HtmlMod.getIdAndOptionsFromElement() functions instead of having to use the addCustomLoadOverride() and addCustomSaveOverride() callbacks respectively. This would allow developers to fix the values earlier, so the other callbacks would just work as expected.
  • The addCustomLoadOverride() and addCustomSaveOverride() callbacks require a return value. See addCustomLoadOverride and addCustomSaveOverride callbacks require a return value #22 for more information.
  • It would also be helpful if the addCustomSaveOverride() function also accepted an individual option, like the addCustomLoadOverride() function does, instead of having to handle every option in the options group at once.

I would be happy to share my code if that would be helpful.

@rugk
Copy link
Member

rugk commented Jul 5, 2022

That all sounds very reasonable and although I don't have that code in mind in detail, I guess yeah… we could change that. Sure sharing your code never hurts (it hopefully lives in some GitHub repo in a branch anyway 😉). Of course, best would be to split this issue into smaller ones, and fix them in this lib one by one then…

@tdulcet
Copy link
Author

tdulcet commented Jul 22, 2022

Sure sharing your code never hurts (it hopefully lives in some GitHub repo in a branch anyway 😉).

Yes, I had not pushed it yet, but here it is now: https://github.com/tdulcet/Bug-Opener/blob/f250b97540093f103a9a4dcd32c9141f9ec06da0/options/modules/CustomOptionTriggers.js#L19-L120

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants