Add segregated combo box field for segregated surface sub-keys#12332
Add segregated combo box field for segregated surface sub-keys#12332RudyTheDev wants to merge 16 commits into
Conversation
| ------------------------------------------------------- */ | ||
| .form-field-input-access, | ||
| .form-field-input-directionalcombo { | ||
| .form-field-input-directionalCombo, |
There was a problem hiding this comment.
While basing this on directionalCombo, I noticed the resulting output has element class form-field-input-directionalCombo but form-field-input-directionalcombo CSS selector name. I wasn't sure if I was missing something obvious here, but a little test with adding color: red; to the selector block showed that it was indeed never applying:
So I renamed it to actually apply. (I guess the alternative is to not append field.type to selector/class name and hard-code it uncapitalized. Or have capitalized variant/type name for all the strings in directional_combo.js.)
As far as I can tell, all this does is makes the caption 1 pixel taller. I am not 100% sure what the block was actually intended to do here.
|
@RudyTheDev please rebase on top of current develop, there are messy, accidental commits in here that we removed by force pushing on develop. Did you see #12005 as well? I was looking into something similar to find a way to expose all kind of sub-tags in a more general way. |
| width: 100%; | ||
| border-radius: 0; | ||
| line-height: 0.95rem; | ||
| align-items: center; |
There was a problem hiding this comment.
While looking at fields with combo box rows, I noticed that the labels were not vertically-aligned. This isn't too bad for access (but which I cannot unsee now) and barely noticeable for directional combos. However while I was adding the custom combo widget, it made a noticeable difference. In any case, I adjusted the selector to make the contents center:
This also re-centered the dropdown triangle, which I think is fine (but we can also make the selector super-specific to just the labels). It now (almost) matches regular combo box triangle location.
| * text=auto | ||
| *.bat eol=crlf | ||
| *.js eol=lf | ||
| *.ts eol=lf |
There was a problem hiding this comment.
I also noticed that Linter is expecting *.ts files to have LF line endings, but .gitattributes doesn't actually specify that, so git will default to local line endings (which in my case meant CRLF and made linter complain). In any case, I added it since it looks there have been a bunch of new *.ts files recently.
@tordans Oh, urgh, I didn't notice develop had a history rewrite. I deliberately didn't want to make a merge commit, so I didn't pull the origin... I'll try to repush, although I've never done something like this onto a cross-repo PR.
Yes, it's one of the issues/PRs I briefly looked at (I haven't pasted all the links yet here). But it's also a much larger generic solution, so I am not certain about the timescale on it as applicable to this. And as you say, it's about discussing the UX options for long-term generic approach, not an implementation (yet). (If you want, I can leave some comments there.) Whereas I'm focusing more on completed code right now since my original issue has been waiting for a long time. I imagine 12005 could theoretically eventually replace this with a bunch of per-field specification. But even this PR took quite a lot of figuring out in terms of various edge cases. So I'd rather have something working sooner (similar to how directionalCombo has been a real life-changer for sidewalks). |
…tive name that never matched
…ombo) for use by cycleway/footway surface
…visibility to show regular surface field when the segregated version isn't shown
…imilarly to directional combo
…onfiguration doesn't override linter expectations
…ging combinations
…d add tests for multi-selection visibility
…e and add tests to check the combinations
…y show when there is a label to fix segregated combo visual glitch
3c754d0 to
923cab7
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Yes, please add comments over there. One advantage of that approach could be, that it does not require changes to the scheme, because it reuses the existing fields and schema for but tags (at least that is the next iteration idea…). Where your change here requires changes to schema-builder and tagging-schema, which both are not trivial to review and implement. Esp. since we just made a schema-builder release today(?). |
|
I just wondered: Can we use the same UI for this situation: Basically for any place, where we have a The UI could look the same. But we would need a different trigger, because there is no And of course we would need to come up with a more generic name than |
I guess theoretically. I think 12005 is closer to what you describe than this. The "problem" is that this implementation is not for a generic type of field - it's only for two extra comboboxes with implied parent value fallback, which is a rather specific case that applies only to some tags and requires minimal fiddling with presets. It's the same sort of assumptions as directional combo. In other words, you could eventually generalize it, sure, but it would create a lot of additional logic to make it more generic against all the assumptions - field type, css, fallback, value, labeling, translation, prerequisite, multi-value, multi-selection, tests, etc. Like, I thought briefly how a generic version would work. But at some point, you would just end up implementing 12005. It was way, way out of scope of what I wanted to add. Even making a small schema change is a whole update process across multiple repos. Just trying to figure out all the repos and setting up local testing was a major pain. It's why I made this targeted PR even knowing that generic solutions have already been proposed, discussed and worked on. My concern is that trying to do it all at once will not get either feature done. So personally, I would support eventually implementing something like 12005 that works for every subkey and then enabling and adjusting it to have preset-specific logic. Like a preset saying |
This PR implements a new field input widget "segregated combo" for combined use by
surface,cycleway:surfaceandfootway:surface.The field is shown and replaces the regular
surfacefield whensegregated=yesis set or if eithercycleway:surfaceorfootway:surfaceare already set.From user's perspective it's still the "same" field, just a different variant that shows extra related values.
Here are the various scenarios:
segregatednot specifiedsegregated=nosegregated=yessegregatedvalueSince this is based on combo box and directional combo box, it should work for RTL by default - I didn't do anything special for it:
Also, when setting the main value, it shows it as empty sub-key placeholders (similar to how access field does it):
I added a bunch of unit tests to check most of these cases and assumptions. In particular, that tag values aren't messed up by the new widget and that widget displays when expected.
It's currently added for "Cycle & Foot Path" and "Cycle & Foot Crossing" presets (though technically "Cycle & Foot Crossing" doesn't use the
segregatedfield for some reason).This PR implements openstreetmap/id-tagging-schema#205 and supersedes openstreetmap/id-tagging-schema#1998 with a more targeted solution.
This PR relies on schema repo update (ideditor/schema-builder#302) and afterwards requires tagging repo update (openstreetmap/id-tagging-schema#2249).
This PR focuses on cycleway surfaces, but this will potentially similarly work for other fields like
smoothness. In fact, this can potentially support stuff likemaxspeed:backwardandlanes:forward(for example, whenlanes=3) and such.I also encountered a couple minor existing issues that I fixed. See comments below.
AI Disclaimer: I used LLM assistance with various code suggestions. I read and checked any such additions or changes and further refactored and updated them manually. All text like this PR was written by me.