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

Add a RowClickUrl href Target Option #83

Open
nickdevore opened this issue Sep 14, 2017 · 4 comments
Open

Add a RowClickUrl href Target Option #83

nickdevore opened this issue Sep 14, 2017 · 4 comments

Comments

@nickdevore
Copy link
Contributor

A user has requested that the RowClickUrl be enhanced to provide a Target option. I see the code that operates on the e.Which at line 302 of griddly.js.

if (e.which == 2 || e.ctrlKey)  
     window.open(url);

I would like to add a RowClickUrlTarget attribute that takes a string to allow us to pass in a value. I would put it in a data-griddly-url-target value on the td and pick it up in the javascript at the same place the url is discovered.

Or, is there a way to do this already (beside the ctrl-click option that already exists)?

@programcsharp
Copy link
Owner

Yeah, we could do that. Submit a PR 😜 If you do, make sure it also applies to the normal clicks.

Looks like here: https://github.com/programcsharp/griddly/blob/master/Griddly/Scripts/griddly.js#L343 -- is your griddly.js out of date?

The other option would be to make a rowclick event, handle that, and put in the logic yourself. I think target is fine for now, but if we need to add more logic than that later, we could go to the event and rip out the custom click/modal stuff.

I've never been particularly happy with the rowclick we have in general. It's non-standard and js, which means that you can't access via keyboard/screenreader or right click to copy the link. Basic issue is you can't wrap a with an which is what's needed to be semantic. Any ideas to make this better in general?

I've seen two approaches:

  • Convert to div/a using display:table-row and display:table-cell so you can use an for a row. This would work but it's a pretty big change. We control all of the rendering though, so it could be possible. And it'd let us do some sweet stuff like tabbing through rows and keyboard selection.
  • Overlay a transparent on the row: https://stackoverflow.com/a/30678005 -- but then you can't click links in columns

@nickdevore
Copy link
Contributor Author

I'll definitely submit a PR. I'll KISS for now and do just the Target piece of this. But I'll keep in mind the rowclick event too. That seems like a good enhancement.

@nickdevore
Copy link
Contributor Author

I just took a look at how the Kendo UI does the select on their grid, and they handle the event, rather than doing anything fancy like a hovering (though that sounded cool). They've got a lot of aria- stuff to help with the screen readers, I guess? I don't think that supports the right click to get the url though. Do you think it would be worth while to replace the right click drop down menu a la gemini ticket tracker? Or perhaps even an icon with three dots stacked on top of each other at the right end that provides options for the row?

nickdevore added a commit to nickdevore/griddly that referenced this issue Sep 14, 2017
@ithielnor
Copy link
Collaborator

Should we consider this solved?

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