New UI for Json field - #2038
Conversation
# Conflicts: # src/UI/src/Fields/Json.php
# Conflicts: # src/UI/dist/assets/app.js # src/UI/dist/assets/main.css # src/UI/src/Fields/Json.php # tests/Feature/Fields/JsonFieldTest.php # tests/Unit/Fields/JsonFieldTest.php
…ove request handling
|
I reviewed this PR and found a few risks that should be addressed before merge:
I also ran PHPStan on the changed PHP classes and it passed. I could not get a valid Pest run from my temporary worktree because the symlinked |
ReviewNice work overall — the new Alpine-based Json UI is a big usability upgrade, the nested rendering is much cleaner than the old TableBuilder approach, and splitting Blocking
BC breaks (4.x)
Non-blocking
Verdict: request changes — items 1, 2 and 4 are user-visible regressions for existing 4.x projects; the rest can be follow-ups. Review by Claude Fable 5 |
# Conflicts: # src/UI/dist/assets/app.js
|
Follow-up review after the fixes on A lot of the previous feedback has been addressed: the public contracts and compatibility methods are back, filter mode and reorderable behavior are restored, the key/value apply heuristic is limited to preview, the schema is memoized, swallowed exceptions were removed, and the deleted feature coverage was restored. The focused unit and feature suites pass, PHPStan reports no errors, and the PR checks are green. There are still a few issues that should be addressed before merge:
My follow-up verdict remains request changes, primarily because the upload and reactive paths are user-visible regressions that the current tests do not exercise. |
lee-to
left a comment
lee-to
left a comment
There was a problem hiding this comment.
очень много странных правок которые не влияют на скоуп Json поля
| relation: $relation, | ||
| resourceUri: $resource ? $resource->getUriKey() : $this->router->extractResourceUri(), | ||
| pageUri: $page ? $page->getUriKey() : $this->router->extractPageUri() | ||
| resourceUri: $resource instanceof ResourceContract ? $resource->getUriKey() : $this->router->extractResourceUri(), |
There was a problem hiding this comment.
а какую проблему тут решаем? он либо нулл либо ресурс
| resourceUri: $resource ? $resource->getUriKey() : $this->router->extractResourceUri(), | ||
| pageUri: $page ? $page->getUriKey() : $this->router->extractPageUri() | ||
| resourceUri: $resource instanceof ResourceContract ? $resource->getUriKey() : $this->router->extractResourceUri(), | ||
| pageUri: $page instanceof PageContract ? $page->getUriKey() : $this->router->extractPageUri() |
| $components = []; | ||
|
|
||
| if ($metrics = $this->getMetricsComponent()) { | ||
| if (($metrics = $this->getMetricsComponent()) instanceof ComponentContract) { |
There was a problem hiding this comment.
а что еще этот метод может отдать?
| && $resource->can(Ability::CREATE); | ||
|
|
||
| $actionButton = $button | ||
| $actionButton = $button instanceof ActionButtonContract |
| }; | ||
|
|
||
| $actionButton = $button | ||
| $actionButton = $button instanceof ActionButtonContract |
| ->withoutWrapper() | ||
| ->setRequestKeyPrefix($parent->getRequestKeyPrefix()) | ||
| ; | ||
| ->setRequestKeyPrefix($parent->getRequestKeyPrefix()); |
There was a problem hiding this comment.
не надо трогать код стайл, после мержа все и так нормализуется а на ревью влияет
| $resource = $request->getResource(); | ||
|
|
||
| if (! $resource) { | ||
| if (!$resource instanceof CrudResourceContract) { |
| $this->prepareJsonFieldsForValidation(); | ||
| } | ||
|
|
||
| protected function prepareJsonFieldsForValidation(): void |
| - '#PHPDoc tag @method for method .+make\(\)#' | ||
| excludePaths: | ||
| - ./src/Support/src/Traits/Makeable.php | ||
| - ./src/UI/src/Traits/Fields/HasVerticalMode.php |
| } | ||
|
|
||
| $casted = $cast ? $cast->cast($value) : new MixedDataWrapper($value); | ||
| $casted = $cast instanceof DataCasterContract ? $cast->cast($value) : new MixedDataWrapper($value); |
There was a problem hiding this comment.
я все еще не понимаю как это все влияет на Json поле
2 participants
What was changed
Jsonfield rendering and nested field handling.Addtext to the Json create button by default.RelationRepeaterinstead of reusing the generic Json field view.RelationRepeaterto render throughmoonshine::fields.relationships.relation-repeater.RelationRepeaterwhen no items are added.RelationRepeatercreate button styling with the Json create button.RelationRepeatercreate button text and icon visibility.RelationRepeater:async(),disableAsync(),isAsync(),getRedirectAfter(), andgetFormButtons().Why?
The Json field needed more reliable nested behavior and a more consistent UI for create actions, empty states, previews, and nested row controls.
RelationRepeaterwas sharing the Json field view, so Json-specific rendering changes could accidentally affect relation repeater output. It also participates in relation handling flows that expect HasMany-like methods such asisAsync(), which could cause forms to fail withRelationRepeater::isAsync does not exist.These changes separate
RelationRepeaterrendering from Json rendering, keep existing non-async behavior intact, and make both fields more consistent with MoonShine UI conventions.Checklist
Tested
Documentation - PR #1034