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

Extension point for cell rendering #5154

Closed
wetneb opened this issue Aug 5, 2022 · 7 comments · Fixed by #5312
Closed

Extension point for cell rendering #5154

wetneb opened this issue Aug 5, 2022 · 7 comments · Fixed by #5312
Assignees
Labels
extension Making it easier to extend OpenRefine's UI and backend Module: Frontend These issues involve working on HTML, CSS, and JavaScript code that affects the user interface. Type: Design Discussions Indicates the need for discussion on issues related to UI/UX, accessibility or design refinements. Type: Feature Request Identifies requests for new features or enhancements. These involve proposing new improvements.
Milestone

Comments

@wetneb
Copy link
Member

wetneb commented Aug 5, 2022

As part of the Wikimedia Commons integration project, there is an interest (@trnstlntk @lozanaross ) in customizing the cell rendering to include thumbnails of the media files being uploaded (or edited): OpenRefine/CommonsExtension#34.

There is currently no official way for an extension to customize how the cells of a particular column are rendered. Therefore, implementing this would first require introducing the corresponding extension point in OpenRefine itself.
Introducing extension points is a subtle thing to do, as we need to care about the following aspects:

  • The introduction of the extension points commits us to maintaining such an interface in a stable way in the future, to some extent. For instance, if we wanted to rewrite the grid rendering in a different framework, this would likely imply changing this extension point, hence breaking extensions which rely on it;
  • The use case that motivates the introduction of the extension point should be generalized sufficiently for the extension point to be useful to other use cases: we need to make a good effort at thinking about in which other use cases one might want to customize the way cells are rendered.

Proposed solution

We first need to enable extensions to store column-specific metadata which controls how the column is rendered. Introduce for this a JSON field in the column metadata object, where extensions can store arbitrary JSON.

Then, add a new extension point which lets extensions register cell rendering callbacks. A cell rendering callback is a javascript function, which takes as arguments:

  • a cell object (as a JSON-deserialized object, which came from the backend)
  • the column metadata for the column in which it is being rendered

The callback is expected to return:

  • either a DOM element, which then becomes the rendered cell value (on top of which is added the "edit" button, still managed by the core software)
  • or null, meaning that the callback is not attempting to override the rendering of this cell and delegates its rendering to other callbacks or the core software

When cell rendering callbacks are registered, they are also attributed a priority (an integer) by the registrant. When rendering a cell, cell rendering callbacks are executed in order of decreasing priority. The first callback that returns a DOM element determines how the cell is rendered. If all callbacks return null, then the core software is used to render the cell.

Alternatives considered

Do not build an extension point for cell rendering, and instead build this feature in the core software, with the understanding that rendering file paths of images as thumbnails is a feature that would be useful to a wide enough community.

Feedback welcome

@elebitzero @tfmorris Can you think of other ways to support this?

@wetneb wetneb added Type: Feature Request Identifies requests for new features or enhancements. These involve proposing new improvements. Module: Frontend These issues involve working on HTML, CSS, and JavaScript code that affects the user interface. Status: Pending Review Indicates that the issue or pull request is awaiting review by project maintainers or collaborators extension Making it easier to extend OpenRefine's UI and backend Type: Design Discussions Indicates the need for discussion on issues related to UI/UX, accessibility or design refinements. labels Aug 5, 2022
@thadguidry
Copy link
Member

thadguidry commented Aug 5, 2022

We could also allow alternative data grid views. Being that the entire data grid is treated as a component and can be switched out by another component. This is typical of modern frameworks. In the case of Commons file previewing, you register the current data view as 'default' key and the alternative as some other key such as 'wikicommons'. This key could be stored as a display preference. Later we might add a gear icon in somewhere in top panel to allow quick option changes to set a preferred view. Extensions could register a custom data grid view component. This has the added benefit of our front end really being more decoupled which I think is healthy for our long term plans.

@antoine2711
Copy link
Member

I think this would be great. I'm all for it, and I think it's something that could improve OR's GUI.

Regards, Antoine

@antoine2711 antoine2711 removed the Status: Pending Review Indicates that the issue or pull request is awaiting review by project maintainers or collaborators label Aug 5, 2022
@wetneb
Copy link
Member Author

wetneb commented Aug 5, 2022

@thadguidry thanks! Deciding at what level the extension point should be is exactly the sort of discussion I expect us to have here.

Making it possible for extensions to swap out the entire grid seems less attractive to me, because that would require re-implementing most of the existing functionality even if you just want to change something marginal. What sorts of uses of such an extension point are you expecting?

Also, you are using the word "component" but note that with jQuery this notion does not really exist: we do not have a well-isolated software component which corresponds to a particular part of the DOM. Also it is not clear to me how that would make the frontend "decoupled".

@antoine2711
Copy link
Member

We could also allow alternative data grid views.

@thadguidry: I understand that would give a lot of flexibility to extension coders.
I would actually like that proposition to show, not a table, but rather a tree representation of the data. Your proposition would enable that.

But I think that it should be an addition to @wetneb proposition, because, as Antonin said, redoing a complete table would be a lot of work.

Regards, Antoine

@thadguidry
Copy link
Member

thadguidry commented Aug 5, 2022

To @wetneb

What sorts of uses of such an extension point are you expecting?

Recon, Graph and Records views (the Epic issue I created) will need many more common components that need to be shared between them, and I've put together a few prototypes that still need work but hopefully I can have more design time on this beginning of next year (in the middle of moving residences).

Also, you are using the word "component" but note that with jQuery this notion does not really exist: we do not have a well-isolated software component which corresponds to a particular part of the DOM. Also it is not clear to me how that would make the frontend "decoupled".

That's not true. jQuery indeed has components. A component in modern web development terms means "a component as a user interface element that only cares about itself." The isolation is already inherently there with jQuery because we are communicating only through public methods and event listeners through each jQuery component itself, which are really big components essentially the *.js files like project.js, index.js, preferences.js This site might help explain our architecture in it's original component context that David built and was thinking about for extension possibilities down the line. https://mitchel.me/2018/writing-javascript-components-with-jquery/

As a small nit, we do have isolated software components within the larger components, they exist as html id's and you can see them in the existing function calls for example leftPanelDiv, toolPanelDiv, viewPanelDiv (which is actually our DataTableView hence data grid) etc. in https://github.com/OpenRefine/OpenRefine/blob/master/main/webapp/modules/core/scripts/project.js

Frameworks such as Vue, Svelte, even Web Components etc. allow progressive integration even with jQuery. For example, with Svelte, the data grid would essentially just be a Svelte app that is rendered into our ui.viewPanelDiv html id. The visualization further down this site might help think about things https://apop.tech/posts/adding-svelte-to-an-existing-website/

Given the above, the nice thing is that jQuery actually helps us natively have extension points already.

@thadguidry
Copy link
Member

thadguidry commented Aug 5, 2022

To @antoine2711

We don't have to redo things completely. jQuery's structure actually already allows componentization. It was a big reason that David chose it at the time. It has its particular way to do componentization, but still it's there and we essentially have 3-4 large jQuery components currently in OpenRefine's javascript files. Components can have sub components... a beautiful thing. The subcomponents can be a Svelte app, a React app, VueJS component, or just a simple vanilla Javascript and html template, etc. We are not blocked by anyone coming into the project and beginning work on alternative subcomponents at any time. It's always been there.

@tfmorris
Copy link
Member

tfmorris commented Aug 8, 2022

Returning to @wetneb 's original question/proposal, I don't have a strong opinion, but would probably lean towards something limited in scope unless there's a strong understanding of the types of cell renderers that people would like to build. As a matter of principle, I think that the existing renderer would need to be modified to use the new extension point, if one were introduced.

Perhaps better to just introduce an optional thumbnail image which would get rendered in one (or a few select) fixed position(s). This reduces the risk of performance degradations, weird extension interactions, etc. Otherwise, what seems, on the surface, like a simple API risks becoming an unintended can of worms.

But, like I said, that's just a intuitive gut feel, rather than a strongly held viewpoint.

@wetneb wetneb self-assigned this Sep 21, 2022
@trnstlntk trnstlntk moved this to To be triaged in Structured Data on Commons Sep 27, 2022
@trnstlntk trnstlntk moved this from To be triaged to ⚠️ WMF/SDC grant (2021-22) - Finish before Oct 31, 2022 in Structured Data on Commons Sep 27, 2022
wetneb added a commit to wetneb/OpenRefine that referenced this issue Oct 1, 2022
wetneb added a commit to wetneb/OpenRefine that referenced this issue Dec 4, 2022
wetneb added a commit that referenced this issue Dec 6, 2022
* Introduce extension point for cell rendering. Closes #5154.

* Fix reference to DataTableCellUI

* Adapt Cypress tests for additional div in the DOM

* Fix more Cypress tests

* Use string-based identifiers for renderers and migrate recon-specific methods to recon renderer
Repository owner moved this from ⚠️ WMF/SDC grant (2021-22) - Finish before Oct 31, 2022 to SDC (WMF) grant 2021-22 - done in Structured Data on Commons Dec 6, 2022
@tfmorris tfmorris added this to the 3.8 milestone Oct 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extension Making it easier to extend OpenRefine's UI and backend Module: Frontend These issues involve working on HTML, CSS, and JavaScript code that affects the user interface. Type: Design Discussions Indicates the need for discussion on issues related to UI/UX, accessibility or design refinements. Type: Feature Request Identifies requests for new features or enhancements. These involve proposing new improvements.
Projects
Status: SDC (WMF) grant 2021-22 - done
Development

Successfully merging a pull request may close this issue.

4 participants