This repository has been archived on 2023-08-20. You can view files and clone it, but cannot push or open issues or pull requests.
symfony/tests/Symfony
Fabien Potencier 4e605aa761 merged branch Seldaek/form_tpl (PR #1519)
Commits
-------

52fdd53 [Bridge/Twig] Add required class to labels that match required fields

Discussion
----------

[Bridge/Twig] Add required class to labels that match required fields

I have used this to simply style labels that are required with a red star behind them using this CSS:

``` css
label.required::after {
	content: " *";
	color: #c00;
}
```

The problem is that you can't use `input[required] + label::after` as a selector since the label is typically rendered before the input. There is no way to check for an element that is *followed by* another, only elements *following*.

Of course this CSS in particular won't work except in the latest browsers, but you could still use the `label.required` selector to add a background image and so on. I think this is a very common use case and therefore I think it'd benefit the core framework.

---------------------------------------------------------------------------

by fabpot at 2011/07/05 01:27:49 -0700

Can you also update the PHP templates?

---------------------------------------------------------------------------

by schmittjoh at 2011/07/05 01:43:33 -0700

How about namespacing these css classes, like for example "sf-form-required"?

---------------------------------------------------------------------------

by stloyd at 2011/07/05 01:50:58 -0700

I would prefer an @schmittjoh naming, or even adding ability to setup it thought options.

---------------------------------------------------------------------------

by fabpot at 2011/07/05 01:54:36 -0700

Please, do not add more options. Prefix with `sf` is actually a good idea but people will argue that this is not a good idea because it gives too much information about the technology used to create the website (that's one of the things that came up pretty often in symfony1).

---------------------------------------------------------------------------

by schmittjoh at 2011/07/05 02:00:11 -0700

An option is not such a good idea imo since you likely want to have a uniform naming strategy across your entire site. How about adding a new service CssNamingStrategy?

---------------------------------------------------------------------------

by stloyd at 2011/07/05 02:01:19 -0700

Then this can be some simpler one not giving such informations i.e.: `form-label-required`, `label-required`, `framework-form-required`, `form-required` or whatever else ;-)

---------------------------------------------------------------------------

by fabpot at 2011/07/05 02:16:41 -0700

It cannot be configurable as it would potentially break bundles that come with stylesheets.

---------------------------------------------------------------------------

by stloyd at 2011/07/05 02:21:10 -0700

IMO if we decide to add this one, we could add same to `inputs/selects/etc` with `required` option.

---------------------------------------------------------------------------

by schmittjoh at 2011/07/05 02:21:59 -0700

I think it can, consider an interface like this:

```php
interface CssNamingStrategyInterface
{
    function getCssName($class);
}
```

This will give people a lot of flexibility, and it also does allow them to exclude classes which for example are provided by third-party bundles.

---------------------------------------------------------------------------

by Seldaek at 2011/07/05 02:47:54 -0700

Wow guys, if this turns into a full blown class generator solution, I'm happy to close the PR.

"required" is not a name that's commonly used for main page elements, it's typically associated with forms, and therefore I don't see the need to make it unnecessary longer/namespaced. Similarly I don't see the need to add it to the input/select/.., because they already have an attribute, which you can very easily select as: `input[required]` in CSS. That works everywhere except IE6, but we can't build for the future on very old browsers. If you really want support for IE6, you can override the templates imo. But core should be looking forward, as it already is with HTML5, form markup, etc.

As for calling it form-label-required or label-required, again, I don't see the benefit, you can use `label.required` if you want to avoid conflicts with non-label elements having a required class, or a safer `form .required`. There are plenty of options in CSS itself, let's not make this overly complex.

---------------------------------------------------------------------------

by schmittjoh at 2011/07/05 02:52:17 -0700

see
http://code.google.com/intl/de-DE/speed/page-speed/docs/rendering.html#UseEfficientCSSSelectors

On Tue, Jul 5, 2011 at 11:47 AM, Seldaek <
reply@reply.github.com>wrote:

> Wow guys, if this turns into a full blown class generator solution, I'm
> happy to close the PR.
>
> "required" is not a name that's commonly used for main page elements, it's
> typically associated with forms, and therefore I don't see the need to make
> it unnecessary longer/namespaced. Similarly I don't see the need to add it
> to the input/select/.., because they already have an attribute, which you
> can very easily select as: `input[required]` in CSS. That works everywhere
> except IE6, but we can't build for the future on very old browsers. If you
> really want support for IE6, you can override the templates imo. But core
> should be looking forward, as it already is with HTML5, form markup, etc.
>
> As for calling it form-label-required or label-required, again, I don't see
> the benefit, you can use `label.required` if you want to avoid conflicts
> with non-label elements having a required class, or a safer `form
> .required`. There are plenty of options in CSS itself, let's not make this
> overly complex.
>
> --
> Reply to this email directly or view it on GitHub:
> https://github.com/symfony/symfony/pull/1519#issuecomment-1502560
>

---------------------------------------------------------------------------

by Seldaek at 2011/07/05 03:11:50 -0700

Really? Come on, we're talking about forms, it's not like you have billions of form/input tags per page that have to be parsed by the browser when you select that. Also you don't have to select the elements, if you want true performance just use no stylesheet, your users will thank you.

---------------------------------------------------------------------------

by schmittjoh at 2011/07/05 03:30:40 -0700

Your CSS selectors not only affect the performance of form elements, but of all elements that have a "required" class. Likewise, the same applies if we decide to add more classes.

Why close the door for people who care about performance? We can easily avoid this by making the css class more specific as suggested earlier. The idea with the renaming strategy is one step further and allows people to "obfuscate" which tool was used to generate the form, or do additional optimizations like shortening the css name.

---------------------------------------------------------------------------

by lenar at 2011/07/05 03:34:34 -0700

@Seldaek: Just for remark I've seen matrix forms spanning multiple screenfuls horizontally and vertically
containing tens of thousands inputs. Not pretty, but they do exist. Basically "poor" man's/company's excel
emulation or something.

---------------------------------------------------------------------------

by Seldaek at 2011/07/05 04:19:13 -0700

@schmittjoh, @lenar: We're catering to the most common use case, for which this will be more than fast enough. Small/medium scale websites don't have to optimize on CSS rules parsing, they usually have much bigger issues to deal with. If you really care about it, overriding the block to remove the class is just as easy as it was to for me to add it, but IMO this is the edge case.

---------------------------------------------------------------------------

by schmittjoh at 2011/07/05 05:37:19 -0700

IMO Symfony should follow best practices by encouraging to use class selectors, and not tag selectors for the reasons explained on the page I linked.

Anyway, I think everybody made his points. Time for @fabpot to make a decision :)
2011-07-11 11:32:26 +02:00
..
Tests merged branch Seldaek/form_tpl (PR #1519) 2011-07-11 11:32:26 +02:00