Open klonos opened 8 years ago
...just came across this again:
...and almost filed a duplicate (had forgotten about filing this issue here). Saved by the auto-complete that revealed this issue's title.
:+1:
Hey @BWPanda ...I tried to fix this in my PR for #2731, but as @jenlampton pointed out, using .field-suffix
is too generic:
This CSS is now specific enough, it will target all suffixes for all fields on this form. If all we're after is the machine name, we should target that single item specifically.
Here's a PR that adds a machine-name-preview
class to the <span>
s around machine name previews, and then targets them specifically: https://github.com/backdrop/backdrop/pull/3069
Instead of repeating the same CSS for each theme, I thought that it'd be best to do it globally, and then leave it up to each contrib/custom to override as/if desired. For now, I have added the CSS to field.css
, but let me know if it should live elsewhere (system.css
perhaps?).
...or do you guys think that it'd be best to have the machine name be rendered below the field instead of to its right (left for RTL)?
It's unclear which is the main PR to review here.
@herbdool For the reasons mentioned in https://github.com/backdrop/backdrop-issues/issues/1349#issuecomment-583655203 I have closed my PR. Please review @klonos' instead.
The PR by @klonos seems to work for me. I tested the two examples provided in the initial description of the problem and looked for other similar pages but could not find any.
@klonos unfortunately your PR has conflicts that need to get resolved. And it really, really lags behind (two years are a long time). So back to needs work.
The milestone doesn't fit the label, BTW. It's marked as "feature request" (why that?), but the milestone is a bugfix release.
The PR still has conflicts, unfortunately. And it hasn't been updated since Feb 2020, I don't think we should keep the milestone, anyway.
This doesn't add any new functionality - it simply improves the way an existing thing works. It's not clearly a bug either, so a task it is (which would justify the milestone just fine, so taking the liberty to restore it).
I have resolved the conflicts and rebased + also fixed a small coding standard issue in an adjacent line in the .css file, which I missed when I initially filed this PR.
This is ready for review/testing once again.
Many thanks for updating - it's so great to have you back. :pray:
I've one concert with white-space: nowrap;
in general - it's not very mobile-friendly. On small screens, if the suffix is long, this may break the layout. One possibility would be to only apply when the screen isn't too small.
But I also have a concern re the hard-coded span
tag: A field suffix might contain block level elements, or a field suffix may contain a closing tag for a field prefix. Here's an example. My suspicion is, that your implementation would break things.
I checked the example form in the sandbox. Yep, the PR breaks markup vadility:
Back to needs work.
Wait a minute... markup already is broken, it's just getting slightly worse now. Do we have an issue for that?
Edit: didn't find any, opened a new one #5735
... A field suffix might contain block level elements, or a field suffix may contain a closing tag for a field prefix. ...
Good catch @indigoxela 👍🏼 ...and thanks for filing that other issue too. I'll see if I can fix things so that HTML tags in pre-/suffixes don't break things.
On small screens, if the suffix is long, this may break the layout.
Right. I have a couple of ideas re that too, but I will need to play a bit with this before deciding. We could try adding white-space: nowrap;
individually to the "Machine name:" label and the actual machine name. Perhaps we should even place the machine name underneath the field instead of next to it. Would that be too radical?
@indigoxela the issue with the invalid markup could be just a matter of using #prefix
vs. #field_prefix
and #suffix
vs. #field_suffix
properly. I often forget that these things even exist, so chances are that others make the same mistake + then people that review code also miss it.
Perhaps we need a best-practice policy, where we discourage using markup in #prefix
/#suffix
and use the respective #field_*
variants for that instead(?) ...or the other way around 🤷🏼
Here's how it works currently:
Here's how it would look if we added
white-space: nowrap;
to thespan.field-suffix
: