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

Make it possible to visually differentiate editable cells from read only cells #72

Closed
Peppe opened this issue Mar 12, 2019 · 7 comments · Fixed by #114
Closed

Make it possible to visually differentiate editable cells from read only cells #72

Peppe opened this issue Mar 12, 2019 · 7 comments · Fixed by #114
Assignees
Labels
enhancement New feature or request
Projects

Comments

@Peppe
Copy link
Contributor

Peppe commented Mar 12, 2019

Now editable cells look like read only cells. There is no different styling, or different pointer. It has no css part defined so that you could theme it yourself.

@web-padawan
Copy link
Member

Not: we could try to add part="editable-cell" for that, by overriding _updateRow method from the Grid and iterating over editable columns to set the attribute:

https://github.com/vaadin/vaadin-grid/blob/77defe3be558b3ba10ce21e7a6c87b38403a5790/src/vaadin-grid.html#L498

@web-padawan web-padawan added the enhancement New feature or request label Mar 12, 2019
@jtomass jtomass added this to 📬  Inbox in vaadin-core Mar 29, 2019
@jouni
Copy link
Member

jouni commented Jan 3, 2020

Would this actually be more of a new state for the cell, rather than a new part?

@web-padawan
Copy link
Member

Whether it should be part or state is an interesting question. See e.g. w3c/csswg-drafts#4412

The obvious distinction is that parts are permanent and part of the identity of the element, while states are transitory and might change.

One problem with attributes is that native ::part() is not working with attribute selectors.
And the :state() pseudo class is only currently supported in Chrome.

Also, we are unlikely to be able to use :state() for it anyways, as our cells are native elements like <td>, and that pseudo class is designed for using by Custom Elements only.

@jouni
Copy link
Member

jouni commented Jan 3, 2020

Right. Didn’t think about how this would play together with ::part() and :state(). Sigh…

I’m still not sure if we are not going to need something like ThemableMixin in the future, even after ::part() and ::theme() are shipping in all browsers. I still don’t understand how you could globally theme a <vaadin-button> without injecting the same style sheet in all style scopes.

My assumption is that you can’t target the host element using ::theme(), so in order to change the background color of all buttons, you’d have to use vaadin-button { background-color: ...; } and inject that into all style scopes.

Alternatively, you need to add a part attribute to all of your buttons across the whole app, i.e. <vaadin-button part="button">, forward those through all shadow roots, and then use ::theme(button) in the global scope. I doubt anyone wants to do that.

Then again, I also think that if you place a component inside a shadow root, you really do want that to be isolated from outside styling and want to opt-in to any customized theming. If a button should be exposed to global theming (apart from custom properties), place it in the light dom.

@rolfsmeds
Copy link

Let's go with an additional part name for now.

@tomivirkki
Copy link
Member

Adding an observer for _cells.* here might be the easiest approach

@web-padawan
Copy link
Member

web-padawan commented Mar 31, 2020

Another way would be to tweak vaadin-grid to expose this line as a method, e.g. like this:

_setBodyCellPart(cell, column , part) {
  cell.setAttribute('part', part);
}

Then in grid pro we could check if column is an edit column, and change a value that we pass to super._setBodyCellPart(). This might be also helpful for advanced users (or CRUD).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
No open projects
vaadin-core
  
📬  Inbox
Development

Successfully merging a pull request may close this issue.

6 participants