Per 8562 indicate fields that are editable #343
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #343 +/- ##
=======================================
Coverage 35.68% 35.68%
=======================================
Files 299 299
Lines 10377 10379 +2
Branches 1719 1719
=======================================
+ Hits 3703 3704 +1
- Misses 6528 6529 +1
Partials 146 146 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Left a question about some of the new inputs introduced in the code. Also, while the ticket mentioned that we might want to include an icon, I will say that adding that icon creates visual inconsistency between the fullscreen and standard metadata editors; the fullscreen one has icons and the "normal" editor doesn't. This is more of a thing for @k8lyn6 to discuss during review but I wanted to bring it up.
| @@ -121,6 +122,8 @@ export class FileViewerComponent implements OnInit, OnDestroy { | |||
| AccessRole.Editor | |||
| ) && !route.snapshot.data?.isPublicArchive; | |||
|
|
|||
| this.canEditFieldsOnFullscreen = this.canEdit && this.canEditOnFullScreen(); | |||
There was a problem hiding this comment.
I'm a bit confused on the purpose of this variable. Is there ever a case where canEdit and canEditFieldsOnFullscreen are going to be different values? I'm not sure if there's a special case but I would think that anything that fullscreen editing shouldn't have different permissions from normal editing (especially since canEdit already checks if the file in question is a Public Archive or not).
There was a problem hiding this comment.
@meisekimiu good point, but the ticket was suggesting adding the text only in fullscreen mode, not in the file list viewer, that's my reasoning. That is why I am making use of the routes. Should I change it?
There was a problem hiding this comment.
Outcome: we'll use canEdit everywhere.
crisnicandrei
force-pushed
the
PER-8562-indicate-fields-that-are-editable
branch
from
4e80aab to
29f05a7
Compare
…an in the html code
crisnicandrei
force-pushed
the
PER-8562-indicate-fields-that-are-editable
branch
from
60c8b24 to
9703c7d
Compare
Steps to test:
1.Open any uploaded file in the full screen viewer.
2. Notice that an icon and Click to add text is present for tags and location (description already had a helper text and the name cannot be deleted.)