Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[ACS-5645] Added edit button to each property panel #8837

Closed
wants to merge 66 commits into from
Closed

[ACS-5645] Added edit button to each property panel #8837

wants to merge 66 commits into from

Conversation

Yasa-Nataliya
Copy link
Contributor

@Yasa-Nataliya Yasa-Nataliya commented Aug 21, 2023 •
edited
Loading

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (check one with "x")

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation
  • Other... Please describe:

What is the current behaviour? (You can also link to an open issue here)
Our current behavior is to have only one edit button which can be used to edit all panels when clicked.

What is the new behaviour?
screen-capture (16).webm

Does this PR introduce a breaking change? (check one with "x")

  • Yes
  • No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:
https://alfresco.atlassian.net/browse/ACS-5645,
https://alfresco.atlassian.net/browse/ACS-5541,
https://alfresco.atlassian.net/browse/ACS-5574,
https://alfresco.atlassian.net/browse/ACS-5575,
https://alfresco.atlassian.net/browse/ACS-5579.

Based on requirements, we've added some new features:

Edit Icon Button: We've added an 'Edit' icon button on each panel. When click this button on a specific panel, that panel becomes editable, allowing you to make changes to its content.

Save and Discard Icons: Once you click the 'Edit' button and make changes to a panel, you'll notice two new icons within that panel: 'Save' and 'Discard.' The 'Save' icon lets you save the changes you've made, while the 'Discard' icon allows you to abandon any unsaved modifications and revert to the original content.

Improved Styles: We've also worked on the styles of property panel. The tags and category panels now have a more appealing look, tailored to match figma design.

Expansion Indicator: we've added expansion indicators to the panel headers.

Edit Button Functionality for Aspect Panel: We've implemented an 'Edit' button functionality for the Aspect Panel as well. Similar to the other panels, clicking this button will enable you to edit the content within the Aspect Panel.

Improved Panel Visibility: we have made significant improvements to the overall layout. Now, all panels are visible without the need for any additional buttons like 'More Information' or 'Less Information.
Alfresco/alfresco-content-app#3414 was a dependency of this PR
https://github.com/Alfresco/alfresco-applications/pull/360 was a dependency of this PR

@Yasa-Nataliya Yasa-Nataliya changed the title [ACS-5645]UI changes for properties panel [ACS-5645] Added edit button to each property panel Aug 21, 2023
@Yasa-Nataliya Yasa-Nataliya marked this pull request as ready for review August 25, 2023 07:38
@AleksanderSklorz
Copy link
Contributor

AleksanderSklorz commented Aug 25, 2023 •
edited
Loading

@Yasa-Nataliya I see a bug with that new design. Earlier when you locked file and then you opened viewer then you could not edit any content inside viewer. Now editing is possible when file is locked. Please have a look at below comparison of new and old design. You can check it on ACA environment.

Screen.Recording.2023-08-25.at.12.55.51.mov
Screen.Recording.2023-08-25.at.12.57.40.mov

@rbahirsheth
Copy link
Contributor

@Yasa-Nataliya I see a bug with that new design. Earlier when you locked file and then you opened viewer then you could not edit any content inside viewer. Now editing is possible when file is locked. Please have a look at below comparison of new and old design. You can check it on ACA environment.

Screen.Recording.2023-08-25.at.12.55.51.mov
Screen.Recording.2023-08-25.at.12.57.40.mov

This issue is resolved.

@DenysVuika
Copy link
Contributor

Focus ring is incorrectly rendered for borders (tested with Edge, FF):

Screenshot 2023-10-10 at 12 16 24

@DenysVuika
Copy link
Contributor

DenysVuika commented Oct 10, 2023 •
edited
Loading

Missing translation key

Screenshot 2023-10-10 at 12 19 41

@DenysVuika
Copy link
Contributor

Incorrect alignment of all form fields vertically

Expected
Screenshot 2023-10-10 at 12 26 37

Actual
Screenshot 2023-10-10 at 12 27 27

@DenysVuika
Copy link
Contributor

Missing gaps between categories

Design:

Screenshot 2023-10-10 at 12 31 33

Actual
Screenshot 2023-10-10 at 12 32 12

@Yasa-Nataliya
Copy link
Contributor Author

Focus ring is incorrectly rendered for borders (tested with Edge, FF):

Screenshot 2023-10-10 at 12 16 24

I tested with Firefox, and it's working fine
screen-capture (20).webm

DenysVuika
DenysVuika reviewed Oct 10, 2023
View reviewed changes
...-services/src/lib/content-metadata/components/content-metadata/content-metadata.component.ts
}

hasTags(): boolean {
return this.tags?.length === 0 && !this.editableTags;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

incorrect logical check... the length === 0 means "has no tags"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have updated the method name.

DenysVuika
DenysVuika reviewed Oct 10, 2023
View reviewed changes
...-services/src/lib/content-metadata/components/content-metadata/content-metadata.component.ts
}

hasCategories(): boolean {
return this.categories?.length === 0 && !this.editableCategories;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

incorrect logical check, length === 0 means "true" but method is called "hasCategories"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have updated the method name.

@Yasa-Nataliya
Copy link
Contributor Author

Missing translation key

Screenshot 2023-10-10 at 12 19 41

I have updated the translation key.

@Yasa-Nataliya
Copy link
Contributor Author

Incorrect alignment of all form fields vertically

Expected Screenshot 2023-10-10 at 12 26 37

Actual Screenshot 2023-10-10 at 12 27 27

Form field styling will come from @AnukritiGL's PR: #8898

@Yasa-Nataliya
Copy link
Contributor Author

Missing gaps between categories

Design:

Screenshot 2023-10-10 at 12 31 33 Actual Screenshot 2023-10-10 at 12 32 12

I added styling according to Figma and did not see any gaps
screen-capture (21).webm

@gl-lovekesh
Copy link

Focus ring is incorrectly rendered for borders (tested with Edge, FF):

Screenshot 2023-10-10 at 12 16 24

Hi @DenysVuika ,
We created the bug to monitor these UI issues in the below ticket.
https://alfresco.atlassian.net/browse/ACS-6108
cc: @AleksanderSklorz

@AleksanderSklorz
Copy link
Contributor

I had a look at this PR. I checked only my comments for now until we decide what we should to do with that PR. Looks like my comments are addressed. Only thing which I noticed is not fully addressed is that there are some accessibility issues found by axe DevTool and not sure if we want to address them in this PR.

image

@Yasa-Nataliya @rbahirsheth @MateuszHY @MichalKinas

@MateuszHY
Copy link

I had a look at this PR. I checked only my comments for now until we decide what we should to do with that PR. Looks like my comments are addressed. Only thing which I noticed is not fully addressed is that there are some accessibility issues found by axe DevTool and not sure if we want to address them in this PR.

image @Yasa-Nataliya @rbahirsheth @MateuszHY @MichalKinas

@MateuszHY MateuszHY closed this Oct 11, 2023
@MichalKinas MichalKinas reopened this Oct 11, 2023
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.6% 0.6% Duplication

@rbahirsheth
Copy link
Contributor

Focus ring is incorrectly rendered for borders (tested with Edge, FF):

Screenshot 2023-10-10 at 12 16 24

@DenysVuika
This is working fine with FF(version- 118.0.2 (64-bit)) and Microsoft Edge(version- 117.0.2045.55 (64-bit)).
Please check the below recording

FF_issue.webm
edge_issue.webm

We are using Ubuntu 20.04.6 LTS OS.
We could not reproduce it for the above-mentioned details of browsers and OS.

@rbahirsheth rbahirsheth mentioned this pull request Oct 17, 2023
Merged
@rbahirsheth
Copy link
Contributor

Closing the PR. We have raised a new PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MateuszHY MateuszHY MateuszHY left review comments

@rbahirsheth rbahirsheth rbahirsheth approved these changes

@gl-lovekesh gl-lovekesh gl-lovekesh left review comments

@DenysVuika DenysVuika DenysVuika requested changes

@eromano eromano Awaiting requested review from eromano eromano is a code owner

@popovicsandras popovicsandras Awaiting requested review from popovicsandras popovicsandras is a code owner

@MichalKinas MichalKinas Awaiting requested review from MichalKinas

@AleksanderSklorz AleksanderSklorz Awaiting requested review from AleksanderSklorz

Assignees
No one assigned
Labels
A/N BDU
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@Yasa-Nataliya @AleksanderSklorz @MichalKinas @rbahirsheth @DenysVuika @gl-lovekesh @MateuszHY @eromano