[MGNLUI-3683] Make LinkField a pure-Vaadin field, move definition handling to factory Created: 30/Nov/15 Updated: 09/Feb/17 Resolved: 10/Dec/15 |
|
| Status: | Closed |
| Project: | Magnolia UI |
| Component/s: | forms |
| Affects Version/s: | 5.4 |
| Fix Version/s: | 5.5 |
| Type: | Improvement | Priority: | Minor |
| Reporter: | Mikaël Geljić | Assignee: | Ngoc Nguyenthanh |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | linkfield | ||
| Remaining Estimate: | 0d | ||
| Time Spent: | 2.5d | ||
| Original Estimate: | 3d | ||
| Issue Links: |
|
||||||||||||||||||||
| Template: |
|
||||||||||||||||||||
| Acceptance criteria: |
Empty
|
||||||||||||||||||||
| Task DoD: |
[ ]*
Doc/release notes changes? Comment present?
[ ]*
Downstream builds green?
[ ]*
Solution information and context easily available?
[ ]*
Tests
[ ]*
FixVersion filled and not yet released
[ ] 
Architecture Decision Record (ADR)
|
||||||||||||||||||||
| Date of First Response: | |||||||||||||||||||||
| Sprint: | Saigon 43 | ||||||||||||||||||||
| Story Points: | 3 | ||||||||||||||||||||
| Description |
|
Just aligning on the field vs. factory pattern |
| Comments |
| Comment by Aleksandr Pchelintcev [ 15/Dec/15 ] |
|
Please DO NOT INTEGRATE THIS ONE UNTIL VERSION 5.5 |
| Comment by Philip Mundt [ 14/Dec/16 ] |
|
I'm not very happy with the description of this ticket and the commit message itself I have to admit. The change brought in by this ticket basically makes any workspaceName set on the info.magnolia.ui.form.field.converter.IdentifierToPathConverter useless and it's basically ignored. This is due to the LinkFieldFactory overriding any workspaceName given with what it finds in its own definition (as targetWorkspace). I agree that this makes sense to some extend but what about the workspaceName of IdentifierToPathConverter? Is it deprecated now? Additionally, the logic involved IdentifierToPathConverter converter = definition.getIdentifierToPathConverter(); if (converter != null) { converter.setWorkspaceName(definition.getTargetWorkspace()); } linkField.setTextFieldConverter(converter); could have been better, no (thinking about the null-check)? Note: I found this only because of |
| Comment by Mikaël Geljić [ 14/Dec/16 ] |
|
PR/commit state exactly what was done, and ticket description gives the rationale: definition handling was moved to the factory. 1-to-1. Really. That said, I understand you were hoping for more from whoever touched this last. |
| Comment by Philip Mundt [ 14/Dec/16 ] |
|
True, it was borked previously already. I created an issue and linked them, will adjust type of link. |