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 extensibility to handlebars, plus some functionality related to format types #417

Merged
merged 8 commits into from
Aug 3, 2023

Conversation

otrok7
Copy link
Collaborator

@otrok7 otrok7 commented Jul 30, 2023

No description provided.

@otrok7 otrok7 marked this pull request as draft July 30, 2023 18:08
@otrok7 otrok7 marked this pull request as ready for review July 31, 2023 11:48

for (var l = 0; l < self.formatsData.length; l++) {
var format = self.formatsData[l];
if (format['format_type_enum'] === "LANG") {
self.uniqueData['languages'].push(format);
if (self.config.native_lang != format.key_string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this have been !==?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@alanb2718
Copy link
Collaborator

Here are a few issues I noticed.

  • The version number needs to be bumped. This should be in three places: two places in readme (stable tag, change log), also in crouton.php. If you're not planning on making a release with these changes, bump the version number and let changes accumulate in the readme until it's time for another release.
  • I tried to test the has_common_needs option, but when I add it to my shortcode nothing happens. How can I test this?
  • What is the purpose of has_meeting_count? There is already a shortcode [meeting_count].
  • Should "Common Needs" be translated in the de-DE section of the localization?
  • How can I test handlebars extensibility?

Ideally there would be a comprehensive set of unit tests, and I could just look at those to check out handlebars extensibility .... but of course we don't have those (yet).

Some documentation would be helpful, so that this isn't another undocumented feature that nobody knows about unless they are willing to read the code. Not needed for this PR though.

Thanks for working on this!

@otrok7
Copy link
Collaborator Author

otrok7 commented Aug 1, 2023

Hi Alan!
I'd rather not do a version bump. I'm not sure these features are even interesting for anyone outside the German fellowship. I'd rather wait till the stuff for meeting detail pages is added. Not much to program there, but I think it's very important.

To test extensions, you have to write a plugin. The plugin can add enqueue javascript that adds partials or helpers to crouton_Handlebars. The helper enrich enriches meeting data, the helper startup can, for instance, add object to the translatable word list. The helpers in partials can then be used crouton column 2 and 3 - and later, in the details page.

I think "common needs" is pretty "eingedeutscht". It is what is used in the definition of FC3 in the root server. NA-speak has a lot of Americanisms, including "meeting" itself (correct would actually be "Treffen", but no one says that). "Gemeinsame Bedürfnisse" similarly sounds funny.

I just tested has_common_needs=1 on my test system, and it seems to work. Would it be possible to debug your page? Maybe send me a link if it's public.

There are a couple of advantages to "has_meeting_count". The first is convenience. Not having to specify the insanely complex query string I need to use just to get all the online meetings - twice. That's a win right there. And then there's performance, since it saves another DB access. Also, because the count is within the bmlt-tabs div, I can CSS it up and put it in the header, as maybe you've seen on the German or Berlin pages. And then I think there's stability issue, since meeting_count and the creation of the crouton object both occur during document-ready, it's possible that they occur in the wrong order. I've seen this when testing crouton.

@otrok7
Copy link
Collaborator Author

otrok7 commented Aug 1, 2023

In fact, do you think it's a good idea to add an "virtual-meetings-only" attribute? It could look up the VM and HY format ids every time the root server is changed, and store them in the options, and then they could be used to generate the query string. Those query strings that avoid the brackets have to be the source of a lot of confusion.
virtual-meetings-only=1 means VM or HY and virtual-meetings-only=-1 means HY or -VM. What do you think?

@jbraswell
Copy link
Contributor

Sounds useful

@alanb2718
Copy link
Collaborator

About bumping the version number: it seems fine to not release a new version with these changes, if you would prefer not to. Instead I'm suggesting bumping the version number on the main branch and documenting the changes in the change log, so that when someone does make a new release, this information is available. Here's how we've been doing this on the root server (and how we used to do it on crouton years ago -- but I haven't done any PRs for crouton in two years).

For version numbers for minor changes that aren't worth a release yet: bump the version number if the last commit was tagged as a release, and let changes accumulate in the readme until it's time for another release. In the changelog, note the release date as "pending" until there actually is a release.

But .... I notice that @pjaudiomv didn't do this for #415 for crouton, and if Patrick isn't doing it, then it can't be a standard thing to do!

What should happen if I have has_common_needs=1? I was thinking it would be like has_formats or has_states, and add a dropdown -- but maybe it does something else?

About adding a virtual-meetings-only attribute -- I agree, this seems useful. "virtual-meetings-only=-1 means HY or -VM" seems a bit obscure, but it would work. What about venue_type instead? Then you could have these options:

venue_type="VM"
venue_type="HY"
venue_type="IP"

or any combination, e.g.
venue_type="VM+HY"

@otrok7
Copy link
Collaborator Author

otrok7 commented Aug 1, 2023

yes, exactly, has_common_needs=1 should create a dropdown. button_format_filters="Common Needs:FC3" creates a view with a button. If you send me link, I can see if the problem is on the php or js side, and if it's on the js side, I can debug it. Otherwise, I don't know if it's a build problem or a bug in the code, or what.

@otrok7
Copy link
Collaborator Author

otrok7 commented Aug 1, 2023

I'm trying to replace the TabbedUI I have working now with crouton, but having the initial query only based on BMLT queries seems too restrictive. I have a page with online meetings in English. So I need two formats or-ed, and a third and-ed. It's probably best to do one of the two and-ed at the root server, and filter the other one (I guess venue-type) in crouton.

@otrok7
Copy link
Collaborator Author

otrok7 commented Aug 1, 2023

And if I do it like that, then your notation really becomes the obvious choice, we can simply allow and format key strings to be added and subtracted. To get all F2F meetings, you could just say "-VM + HY". The thing is, I don't really think HY is really useful as a meeting list type. I think people either want to see all meetings, or they want only to go to a F2F meeting (and don't care if the meeting is also hybrid) or want to go online (and don't care if the meeting is also F2F). But I suppose your notation is the most flexible.

@alanb2718
Copy link
Collaborator

Sorry, false alarm about has_common_needs=1 ! I got it working.

Regarding hybrid meetings, I agree, it's not clear this much flexibility is needed. The existing crouton has a "Venue Types" dropdown, with just two possibilities: In Person and Virtual. Hybrid meetings are included in both. If we wanted to match that, then venue_type="IP" and venue_type="VM" would be the only two options, and would include hybrid in each. I don't have a strong opinion one way or the other about this; but these alternatives seem cleaner than a 1 or -1 arg.

@otrok7
Copy link
Collaborator Author

otrok7 commented Aug 2, 2023

Okay, I added shortcode attributes 'venue_types' and 'formats'. Both take the codes used as used in root server queries. It's possible before the next release to use the string codes, but seeing as how I would have to invent one for "in-person", it didn't seem worth it. So it expects a comma separated string of (possibly negative) integer values.

@otrok7
Copy link
Collaborator Author

otrok7 commented Aug 2, 2023

I also added a "none" css theme. We have a designer doing our CSS, and he knows how to enter his design into our wordpress Theme. It would only add confusion to ask him to make his changes in the Crouton plugin.

@otrok7 otrok7 merged commit 77794a1 into main Aug 3, 2023
1 check passed
@otrok7 otrok7 deleted the Add-Extensibility-To-Handlebars branch August 3, 2023 19:22
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.

3 participants