Adjust collapse and display templates' html. Was intended for plans I…

[plocket]

… have for ALKiln to more easily test these elements (using a container with an id), but I found that to make those changes other changes were needed, and along the way I ended up changing the html a bit more than I expected. I hope it's more robust and accessible to manipulation (like for CSS). The most "breaking" change is that the collapse template title can no longer appear to sit inline with the text surrounding it, but that's probably not a terrible thing because that feature only works when the element is at the very end of text. If it's in the middle of some text, the template content will end up pushing the rest of the text down unexpectedly. I also noticed that the copy button template didn't show the subject of the template unless it was also being collapsed, so I adjusted that too.

This was really supposed to be just a quick tweak and has dragged out a bit. Some justification for changes:

When I have sibling elements that belong to the same functionality, it has helped me to have a container with a class and an id for access by css and js. It's really the most helpful place I've found for a class or id because everything in it can then be accessed. For that reason, if we ever get a chance to re-do this or run into similar situations, I'd rather reserve classname for the outer-most element.

Similarly, I've found it helpful to avoid loose text. Loose text has at times made css and js access complicated or impossible. I prefer it in a container (span or div usually) when possible.

class_name vs classname: the display and collapse template functions each had their own spelling. I want to reconcile them and stay backwards compatible. I went with classname because that let me do container_classname which looks better to me for some reason, but we can switch it up if folks want. I went with container_id_tag to be consistent with container_classname. [I had to use container_ in one function because classname already existed, so I used it in both functions for consistency.]

Icon class: The expanding/collapsing icons can end up being different sizes. To let users style them to be a consistent width, I gave them a shared class.

subject class: I wanted to have a way to access the subject text without relying on tags (the h3, which I want to change - #173 - though I'm not sure what useful semantics would be).

Additional css for display template: It also seemed that fields were being used to create spacing, not to set variables. Without a container, I can see why that was needed. Now it's handled with css.

[nonprofittechy]

Not sure what information container_ prefix is adding to the parameter names.

[BryceStevenWilley]

What's the movitation for allowing people to set the container id directly, or why can't it just always be { the_id }_container?

[plocket]

Because that way they can use the id in ALKiln to detect elements (in a future feature) without worrying about changing their variable's instance name.

[BryceStevenWilley]

I'm not sure that would be useful, IMO, given that it doesn't hold for any other time you change variable names in an interview. I think it's more straight forward to say "if you change variable names in your interview, you'll need to change those variable names in your tests as well", instead of trying to give tools that help you avoid it in some (relatively small cases) but not others.

[plocket]

Another reason is so that someone can use the id for styling or js consistently and not worry about instance name changes.

[plocket]

See lower down for more responses to this.

[plocket]

Not sure what information container_ prefix is adding to the parameter names.

Sorry, I tried to explain, but obviously didn't manage to describe clearly. Since classname already existed to give the template contents a class, I had to distinguish the container classname from the content classname, so I did. The rest of the names with the container_ prefix were staying consistent with that.

[BryceStevenWilley]

Actually had time to fully review:

I like most of this PR! Fully support adding container divs and classes to more things, and +1 on migrating to classname as a param over class_name.

I am hesitant to add a lot of parameters to customize the classes and ids though. You mentioned your reasons for doing so, but I'm afraid the complexity isn't likely to be used, and isn't the right tool for the job; they'll already have unique ids and classes, and if people want to style the same widget different ways (something I'd discourage people from doing, since it would be confusing visually to see two differently styled elements that have the exact some functionality), then I'd suggest they customize it based in the already unique id.

[BryceStevenWilley]

<p> isn't allowed to contain <div>s, it's not valid HTML (https://stackoverflow.com/a/9852381). I'd change this to be a div.

[plocket]

[Yeah, that occurred to me, but there was a styling issue (which is a dumb reason, but maintenance cost is a thing.)] If we change the p to be a div then the markdown paragraph spacing disappears and we have to handle the spacing ourselves. Thoughts?

[BryceStevenWilley]

Isn't it already there? You give al_display_template a bottom margin of 1rem, which is the same spacing we give to paragraphs?

[plocket]

This is for the collapse template, not the display template.

[BryceStevenWilley]

Gotcha, missed that. Should there be a difference between collapse and display template (when collapsed) though? I don't see any problem with adding the same 1 rem bottom margin to al_collapse_template.

[plocket]

Thinking about it more, I think you're right about p, but I will add this note:

Trying to style the spacing ourselves (including the way I'm doing display_template now come to think of it) can set up complications for our users later. If our users decide they want different spacing for their paragraphs, they then have to notice our elements are behaving differently and then make special rules for them.

I'm not sure how to solve it, though. I couldn't find anything using a css variable for the p spacing that we could also use for the div spacing. I can't think of any systemic way to handle it.

[BryceStevenWilley]

A thing that was already present, and just being expanded on: if there's no subject, there will be an empty h3. We should instead just not generate a subject element, or maybe make it a div instead.

[plocket]

I'd suggest they customize it based in the already unique id

The current unique id is only on the content, it won't be able to control the style of any of the other elements.

[BryceStevenWilley]

The current unique id is only on the content, it won't be able to control the style of any of the other elements.

If you don't pass in the container_tag_id, this PR makes an unique id for the container. I guess I don't see the usefulness in making a container id that's completely independent of the content's id. Realizing I would have been more correct to say "the unique ids" instead.

[plocket]

Sorry I'm not expressing things clearly.

If you don't pass in the container_tag_id, this PR makes an unique id for the container. I guess I don't see the usefulness in making a container id that's completely independent of the content's id. Realizing I would have been more correct to say "the unique ids" instead.

"making a container id that's completely independent of the content's id" - Are you saying we should reduce to one id instead of both and just put it on the container? If so, I'm not sure what people are currently using the contents id for. Will it break their stuff to move it to the new container? If not, I'm not sure what you're referring to here. Maybe better to have a real-time conversation.

but I'm afraid the complexity [of custom ids] isn't likely to be used, and isn't the right tool for the job; they'll already have unique ids and classes, and if people want to style the same widget different ways (something I'd discourage people from doing, since it would be confusing visually to see two differently styled elements that have the exact some functionality), then I'd suggest they customize it based in the already unique id.

I think connecting the variable name and the id name is too coupled and too fragile. If I were using it that way, I couldn't rely on that id for anything, like css or js, unless everyone editing the code was aware that they can't change that variable name. I think the options are to either remove the ids id (if we feel they're it's not going to be used), only add them it if they are it is a custom value, or (if we want to have backwards compatibility) to at least allow for customization.

I'm not sure that [using custom ids for elements] would be useful, IMO, given that it doesn't hold for any other time you change variable names in an interview.

New thoughts on this:

1. I believe it's what we're doing for tabs, right? Maybe I need to go back and check.
2. This would be a very explicit And an element with the id "blah" is on the page. I'm not sure what sentence could describe an id generated based off of an instance name. And an element with the id with the name of the variable "blah" is on the page? We'd add documentation for the new Step, of course, that described customizing the id under various circumstances.

Related question: Do templates always just have a simple variable name? That is, they're never something like users[0].the_template or templates[1]? If they can be compound names like that, does our method of making the id that way work? If we need to account for that, would ALKiln then need to mimic the new way we decide to make the id string safe?

[plocket]

[Unrelatedly,] We probably want to try to, at some point, see if we can have display_template use collapse_template instead of doing its own thing. Might be able to use the collapse_template's container_classname for that.

[Other thoughts: Abstract some of these pieces so both collapse_template and display_template can compose what they need when they need it.]

[nonprofittechy]

These do all seem like good changes.

1. Agreed with Bryce that having a single ID should be sufficient.
2. container_classname and container_id_tag feel too long to me. If we have one custom id I think we don't need those long parameter names, right? Keep in mind people can always use their own bootstrap component directly when they want to go wild with customization. (although I recognize people often don't do that)
3. Just want to make sure we don't break signature compatibility, especially with display_template() as that has wide usage outside of our core Monday calls. I don't think there's a lot of customization of the styles for that input in the wild, other than changing the symbol from > to +

[plocket]

a single ID should be sufficient

Do you want me to

1. remove the new one or
2. keep the new one and remove the old one

[nonprofittechy]

a single ID should be sufficient

Do you want me to

1. remove the new one or

2. keep the new one and remove the old one

It appears to me that we can add a new id on the container and remove the old one safely. This is based on projects that I've seen or worked on where this component was used just as-is without any targeting of the id.

[plocket]

Content id tag: unfortunately it looks like the content id tag is, at least in display template, being used to connect the subject to the content. Right now as the href for the link, but I assume in the future that (unless we use detail/summary) we'll need some similar way to link the two.

Having two ids in there isn't terrible, I think, since if we do end up concluding that they both have uses.

[plocket]

Also, I'm reluctant to say it, but instead of transitioning to classname, maybe it should transition to content_classname to match container_classname. I suspect we don't do it, but I dislike the inconsistency slightly more than I dislike the name.

[plocket]

I suspect some things got lost in the shuffle. Still would appreciate thoughts on the new id and on the validity of the current id creation. Also on the length of the names of the new params as I'm unclear if I've addressed those sufficiently.

[BryceStevenWilley]

Sorry for the long one, there were a lot of questions, trying to answer them as comprehensively as I can here;

Are you saying we should reduce to one id instead of both and just put it on the container? If so, I'm not sure what people are currently using the contents id for. Will it break their stuff to move it to the new container? If not, I'm not sure what you're referring to here.

I'm suggesting that you should not have to pass in an id. Inside the function, the code to make the content id and the container id then looks like this:

    the_id = re.sub(r"[^A-Za-z0-9]", "", template.instanceName)
    actual_container_id_tag = f"{ the_id }_container"

Nothing gets broken, because nothing about the content id changes. The container id is brand new, and will always be different than the content id, so it won't break anything either.

I think connecting the variable name and the id name is too coupled and too fragile.

Changing my views on this is a small way after reading all of the discussion here; I now agree that using the variable name as an id for CSS styling is too fragile. However, like Quinten said, "people can always use their own bootstrap component directly". They could even wrap the collapse template in a div with whatever class they want in the markdown, since you can use HTML in markdown. And all this is only for when people want to style the toggle two different ways in the same interview, which I don't think we want to encourage.

I still don't think linking the ids to the variable name is too coupled and fragile for the ALKiln tests. I forgot, but you are right; we are already doing this for tabs, and actually we have a step that clicks on a arbitrary element on the page found with an element selector, which might be really similar to your "And an element with the id "blah" is on the page" step. The tab elements is actually setting it with subject strings (even worse!), but I haven't had any particular troubles with it. We can document the step "the id name will be <variable_name_with_underscores>_container", etc. And I don't think it's that bad to be coupled to variable names, when every other step in the tests are still coupled to variable names. Removing the coupling here doesn't practically help testers when they change a lot of variables names; they'll always have to change the tests as well.

Do templates always just have a simple variable name? That is, they're never something like users[0].the_template or templates[1]? If they can be compound names like that, does our method of making the id that way work?

They can be anything; I have a few templates attached to DAObjects in the EFSP stuff. I think the way we turn them into ids is probably fine; MDN suggests that they stick to ASCII and numbers only. Dots still work, but they have to be escaped when using in selector query stings so they're not parsed as classes by the selector query. It's very unlikely (and probably discouraged) to have two different variable names that get turned into the same id, like users[0].the_template and users0thetemplate, both on the same screen.

[plocket]

I still don't think linking the ids to the variable name is too coupled and fragile for the ALKiln tests.

They can be anything; I have a few templates attached to DAObjects in the EFSP stuff. I think the way we turn them into ids is probably fine

This auto-id making with processing means that ALKiln has to also process the ids in the same way, but only for display templates as other ids on the page could have other characters like dashes or underscores.

[plocket]

As per deep dive notes:

We’ll use a single id and auto generate one for the “content” from the one for the main container.
We need to encode the ID in base64 if we allow authors to provide arbitrary characters.

Reasons for replacing our current method of encoding with base64 encoding: it will be less likely to run into name conflicts and will match up with how docassemble handles this.

I think that's what needs to be done now, but it was so long ago I'm not sure if there were other items that we discussed earlier that weren't captured during that conversation.

[plocket]

Actually, what did we decide about container_classname and content_classname [as keyword arguments]?

[plocket]

Does class_name belong around the content as opposed to surrounding both the title and the content? The element currently using class_name has the container classname already.

[BryceStevenWilley]

Actually, what did we decide about container_classname and content_classname [as keyword arguments]?

My recollection was that we didn't need them at all. Mostly because we thought that if some one wanted very specific CSS control over their collapse / display templates, they can use their own bootstrap components, and don't need to use our function to make that bootstrap. An earlier note in the deep dive notes says:

We’re going to make it less customizable for every CSS class so it’s simpler to use

[plocket]

Somehow I merged by [force] pushing a rebased version [- I was resolving conflicts]. Let me know if we need further changes.

[plocket]

Or maybe I didn't merge. I'm a bit confused now.

[nonprofittechy]

Closing, replaced by #187