Proposition for a more controlled facet mechanism #3717
Conversation
2020-04-19 Update master
First implementation, is functional.
Made the string i18n for title and hover.
Added columnName for hover, a shema to display source when it's not a column (i.e. <internal-flags> for flags facets, and $.attr() calls for i18n string that were already there.
Adding _editTitle()
Add the call click().
Adjusting facetTitle bind property of the facet.
Adding style for FacetTitle in facets.less
i18n core-facets/facet-current-title
cursor: text
Put the name of the column.
Change facetTitle to titleSpan.
Cleanup facetTitle, uniformize it use in facets.
Space cleanup
Fix cursor: move; in facets.less
Adding column name with « this._config.name ».
Use this._config.columnName instead of this.name. Do nothing if new name is empty.
Entering an empty name will restore column name.
Adjust source name for non-column facets by adding braces (< and >).
Saves much more informations regarding the configs with the permalink.
|
|
||
| if(this._config.source == "" || this._config.source == null) | ||
| this._config.source = config.columnName; | ||
|
|
There was a problem hiding this comment.
I am a bit confused by these new fields - I am not sure why it is necessary to introduce them? If we add them, doesn't that mean we should also add support for them in the backend (for instance so that those configuration fields appear in the JSON version of the history)?
There was a problem hiding this comment.
@wetneb : if the user modifies the name of the facet, and then wants to get back the original name, we must save it. The source is an indicator to help the user to know what this facet is about.
I know this add quite some characters to the permalink, and that there is a limit on the URL length, but still, I believe it's worth.
I'll check for the history.
Regards,
Antoine
There was a problem hiding this comment.
I am not sure why it is useful to let the user restore the original name to be honest. I'd just keep it simple and only store the current name. For instance, for projects, we don't store the original name either, and it works just fine.
There was a problem hiding this comment.
@wetneb : I did that (add the original name and the source in the JSON of facets) because it's the way that the permalink will get it. It I want to retain both the changed name and the original name, I had to do this (add it to the JSON).
But, I will change that to just a ID, numerical, and I will be able to restore both new info I added. It will be clean and short.
Regards,
Antoine
There was a problem hiding this comment.
Thanks! I think we should be able to implement this without adding any new field to the JSON representation of facets.
There was a problem hiding this comment.
Thanks! I think we should be able to implement this without adding any new field to the JSON representation of facets.
@wetneb : this new code doesn't add the new facet ID to GetJSON(), so it will not go to the server and/or the operation history.
But the facet parent class has a new GetJSONByID(), and Permalink are build with this method, so a page loaded by a permalink will retain the original name most of the time.
This does all what you asked.
Regards,
Antoine
Only send ID for facets to Permalink, not to the server engin in the operations.
There was a problem hiding this comment.
I am a bit worried by the turn this is taking - this seems to be very far away from what I would expect. It would be worth discussing more before you make complicated changes like this.
I don't see why we could rely on numerical ids like this - note that we need to support custom facets with arbitrary expressions. Introducing numerical ids creates a whole lot of issues (how do we ensure stability of those ids? How can extensions define facets with new ids without conflicting with existing ones?).
We really should have a proper conversation asap before you invest more energy into something that will not be mergeable at all.
antoine2711
changed the title
|
|
||
| if(this._config.source == "" || this._config.source == null) | ||
| this._config.source = config.columnName; | ||
|
|
There was a problem hiding this comment.
Thanks! I think we should be able to implement this without adding any new field to the JSON representation of facets.
@wetneb : this new code doesn't add the new facet ID to GetJSON(), so it will not go to the server and/or the operation history.
But the facet parent class has a new GetJSONByID(), and Permalink are build with this method, so a page loaded by a permalink will retain the original name most of the time.
This does all what you asked.
Regards,
Antoine
@wetneb : yes, I was aware that it was going in the wrong direction. I still had to code it, and show it to you. But I will simplify all this very much, and putting aside the ID concept for facet. I wanted to go that far… just for the pleasure. Expressions are taken in account. For the stability of ID, it would be a matter of just adding them with a new incremental ID. For extensions, I was thinking of a 4 bytes hash, CR16, * 256, so this gives a numerical space of 250 facets for the extension, with little chance of colliding with other extensions. Don't worry, I'm not wasting anyone’s time with this branch anymore, and I am coming back with something you’ll like, much simplier. I already knew before committing that it was not mergeable material. I still believe the exercice has some merits. Regards, |
|
If this PR isn't meant to be merged perhaps we could close it? |
Proposition for a more controlled facet mecanism.
Changes proposed in this pull request:
Does just like « project renaming », but for facets, with lots of code to be able to recreate the original name.
A more simplified PR will be submitted.
Regards,
Antoine