Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Standardise element ID hook attribute #21

Open
sfcgeorge opened this issue Jun 15, 2018 · 4 comments
Open

Standardise element ID hook attribute #21

sfcgeorge opened this issue Jun 15, 2018 · 4 comments

Comments

@sfcgeorge
Copy link

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
Copy link
Member

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
Copy link
Author

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
Copy link
Author

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
Copy link
Member

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

  • the flow_id attribute still shows up in production markup. It would be nice if this was gone since it's only used for testing, but that would tightly couple Flow and the Lucky HTML DSL requiring Lucky to understand that when it sees it, and it's not Env.test? then hide it.
  • Using el("@my-taco-stand") is no shorter or more concise than el("#my-taco-stand").
  • Doing CSS selectors to find an element is pretty common for web developers. Anyone using web scraping tools, or javascript, or coming from other similar libraries are already accustomed to using CSS selectors. Adding a new concept also means an extra learning curve. The flow_id works exactly the same as an id attribute in that it should be unique on the page, but my app is currently riddled with duplicates because for some reason, none of us put that together 🤷‍♂
  • It's extra noise added when writing out specs. As I write my flow spec, then I'm having to go and update pages and components all over to make my spec work.
  • The last thing is that id is actually more flexible. You can do el("#my-taco-stand > a.link"), but you can't do that with a flow_id since it would be converted to [flow-id="my-taco-stand > a.link"].

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants