luckyframework / lucky_flow

Automated browser tests for web applications. Similar to Ruby's Capybara.
https://luckyframework.github.io/lucky_flow/
MIT License
51 stars 8 forks source link

Standardise element ID hook attribute #21

Open sfcgeorge opened 6 years ago

sfcgeorge commented 6 years ago

I like that Flow encourages using a custom attribute (flow-id) rather than regular id for selecting elements, as it decouples logical selection from CSS styling selection, thus should be more flexible and less brittle. But flow-id seems quite project specific as an attribute name (and invalid HTML markup).

  1. I think it should be customisable in config for edge cases / preference. To keep existing behaviour you'd set config.hook_attribute = "flow-id".
  2. The default perhaps could be changed to something more standard / compatible.
  3. The attribute would ideally be prefixed with data- to be valid markup.

Unless the HTML spec has changed to allow this, defining a custom attribute like flow-id is invalid markup. HTML5 provides data attributes as a valid way to do this. Angular naughtily supports ng-* attributes thought they also allow the valid data-ng-* form. There is obviously a brevity tradeoff, but I feel a framework should encourage the best practice, while optionally allowing alternate configuration to taste.

Adding an "ID-like" attribute for programatically selecting elements (rather than stylistically) seems like a common thing to do. You might do it in JS for logic. You might do it in other spec frameworks. One other example I know is Deface; a Ruby tool for programatically rewriting views. Deface doesn't have or enforce any custom-ID shortcut but Spree (which uses Deface heavily) has settled on data-hook as the preferred way of selecting elements. Perhaps LuckyFlow could use data-hook by default? Are there other tools that use a different data-* attribute for similar purpose?

FWIW I'm starting to use data-hook in my Rails Rspec Capybara specs.

paulcsmith commented 6 years ago

Thanks for the issue and especially for the thorough examples.

I like the idea of making it configurable so people can use whatever they like!

I'm not sold on data-hook because it seems a bit too ambiguous. On the flip side, that ambiguity means it can be used in multiple ways as you suggested (js hook, deface, etc.).

I think by default though, the ambiguity makes things less clear. Allowing you to configure things if you have a pattern you like, or that another tool supports allows the best of both worlds.

Also, even though technically you must add data- before attributes, it's a very loose suggestions and all browser work fine without it. This is one of those things where I'm not sold on the usefulness of it. Dusk, Vue and Angular don't use the data- and I'm sure there are more.

I'm ok leaving it as flow-id but allowing configurability if having data- is important to you. However, I'd be willing todo data-something if the name is clear and short since it will be used fairly often.

Maybe data-flow? Though that kind of sounds weird. I like that flow-id is super specific.

I feel this is a good balance between letting people choose and having a clear and succinct default. What do you think?

sfcgeorge commented 6 years ago

Thanks for the thoughtful response.

I like brevity and nice looking code too. If "invalid" custom attrs work fine and are already widely used then that's a pretty strong argument.

I also like reliable code and minimising risk of future issues such as name collisions. But really a collision could happen either way, it's hard to predict what combinations of libs will be used.

data-flow sounds more like something to do with data flow :P

data-id seems quite a descriptive option to me, but also seems so generic other libraries might already use it so could collide.

My takeaway would be I don't see a clear "best" default, they're all good to me. Configurability would allow personal preference and fixing collisions with specific combinations of libraries. Perhaps stick with flow-id and add the config.

sfcgeorge commented 6 years ago

Ooh this is a nice article on the issue of using IDs for stuff other than styling. They also went with data-hook (that's what I searched for) https://ampersandjs.com/learn/data-hook-attribute/

jwoertink commented 4 years ago

After spending some time in Flow, I'd actually like to propose getting rid of flow_id. My main reasons are these:

I think one argument that article makes against using id is that it would technically be possible to use 2+ elements with the same id which could break your javascript. We run in to the same thing using flow_id because we don't have a way to detect it being used more than once.

On the plus side, the one thing I do like about them is that if you move that element somewhere else on the page, your specs would still pass if written correctly. Aside from that, I find myself wanting to use them less.