Conversation
Koc
force-pushed
the
feature/add-relations-column-type
branch
from
c3f6a2d to
fdd744e
Compare
Koc
force-pushed
the
bugfix/fix-loading-of-large-tables
branch
from
f792683 to
f599f40
Compare
Koc
force-pushed
the
feature/add-relations-column-type
branch
6 times, most recently
from
92af03c to
0f9416b
Compare
Koc
force-pushed
the
feature/add-relations-column-type
branch
3 times, most recently
from
6a4e073 to
5209182
Compare
Koc
force-pushed
the
feature/add-relations-column-type
branch
4 times, most recently
from
fb98364 to
198afa1
Compare
Koc
force-pushed
the
feature/add-relations-column-type
branch
from
198afa1 to
ddc1bd1
Compare
Koc
force-pushed
the
feature/add-relations-column-type
branch
2 times, most recently
from
f34f8f9 to
d406233
Compare
Koc
force-pushed
the
feature/add-relations-column-type
branch
4 times, most recently
from
f092bad to
502b69b
Compare
Koc
force-pushed
the
feature/add-relations-column-type
branch
from
502b69b to
8093d63
Compare
Koc
force-pushed
the
feature/add-relations-column-type
branch
7 times, most recently
from
d45b95c to
cb637bd
Compare
enjeck
force-pushed
the
feature/add-relations-column-type
branch
from
cb637bd to
9b36f43
Compare
enjeck
left a comment
enjeck
left a comment
There was a problem hiding this comment.
Thanks, it works quite well already. A few comments for now. I have only tested the creation of columns and rows so far
| }) | ||
| this.availableLabelColumns = columns | ||
| .filter(column => | ||
| column instanceof NumberColumn || column instanceof TextLineColumn, |
There was a problem hiding this comment.
why restrict to just these 2 column types?
There was a problem hiding this comment.
it seems like the relation should easily generalise to all column types
There was a problem hiding this comment.
In theory we can, but I'm not sure that this is good idea. This is the column which we will display in the relation dropdown. So, this column should be unique in order to properly select correct relation row (otherwise you will select wrong rows).
What sense to display select/multi-select/rating here? I doubt that those type of columns will be unique.
| $targetColumn = $this->columnMapper->find($settings[Column::RELATION_LABEL_COLUMN]); | ||
| if ($isView) { | ||
| $view = $this->viewMapper->find($targetId); | ||
| $rows = $this->row2Mapper->findAll( | ||
| [$targetColumn->getId()], | ||
| $view->getTableId(), | ||
| null, | ||
| null, | ||
| $view->getFilterArray(), | ||
| $view->getSortArray(), | ||
| $this->userId |
There was a problem hiding this comment.
Do we ever consider permissions such that the user must have access to the table/view before they can link to it?
There was a problem hiding this comment.
No, and this is a "feature by design". The main idea is that admin creates tables for dictionaries, e.g. users, departments, offices, etc, then creates table that references to this dictionaries, e.g. vacations. And he can share only vacations to the end users.
Otherwise he should to share all dictionary tables as well. And users will have tons of irrelevant tables in their shares. Or admin can forget to share dictionary tables and users will suffer because table with relations not displays all columns.
We can't share table partially per column right now. Let's preserve same approach for new column types as well.
Koc
force-pushed
the
feature/add-relations-column-type
branch
2 times, most recently
from
7e22314 to
b47617a
Compare
Signed-off-by: Kostiantyn Miakshyn <molodchick@gmail.com>
Koc
force-pushed
the
feature/add-relations-column-type
branch
from
b47617a to
3240639
Compare
|
So I have user tested and this looks good. 👍 Could not get it to work at first because I did not get that only text field were working. This will create maybe some confusion on first use. Not all tables do have this text field. Perhaps a comment with a small tooltip with a little (i) in the screenshot below to explain that you can only link that kind of data For my mention of #2035 this is not a good fit. |
@Aveyron-RetD yeah, I'm aware about that case. But you need to understand me as well: feature is really huge, we should start from something simple and improve it in upcoming iterations 🤝 I will add tooltip soon. Hope that we can merge it soon 🙏 |
5 participants
This PR introduces a basic relation type. It allows to connect few tables together. We will store row id as a column value and display selected column as a label for this values. Closes #172.
✔️ Implemented
🔍 Column create/edit
🔍 Row create/edit via popup/inline
🔍 Row display in table/view + handle relation row removal
🔍 Filter for view
🔍 Filter for rows
🔍 Import/Export
🚧 Pending (help welcomed):
❎ What is out of scope: