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.

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

[MNT-23433] Close Preview button position #3535

Merged
merged 11 commits into from
Jan 3, 2024

Conversation

AnukritiGL
Copy link
Contributor

@AnukritiGL AnukritiGL commented Nov 23, 2023

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)
The button to close the preview is located on the left side while the other control buttons are on the right.

What is the new behaviour?
The button to close the preview is now configurable, customer can set the position of close button either left or right.

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/MNT-23433
Dependent PR: Alfresco/alfresco-ng2-components#9143

@AnukritiGL AnukritiGL marked this pull request as ready for review November 23, 2023 07:19
@AnukritiGL AnukritiGL force-pushed the MNT-23433-preview-buttons-positions branch 3 times, most recently from dd3902d to 0e09321 Compare November 27, 2023 06:34
@@ -1132,6 +1132,7 @@
"downloadPromptDelay": 50,
"downloadPromptReminderDelay": 30,
"enableFileAutoDownload": true,
"fileAutoDownloadSizeThresholdInMB": 15
"fileAutoDownloadSizeThresholdInMB": 15,
"isCloseButtonOnLeft": false
Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to make button position configurable I think that this param should accept right or left values and also I think the name of the param should be adjusted

Copy link
Contributor

@MichalKinas MichalKinas Dec 8, 2023

Choose a reason for hiding this comment

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

This comment is unfixed still, I also mentioned the same improvement in ADF PR

Done

@@ -1177,6 +1177,23 @@
}
}
]
},
{
"id": "app.viewer.separator.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Ids of elements should be unique

"order": 11000
},
{
"id": "app.viewer.close",
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should add new button for that, the close button already exist in ADF and ACA should just set the position value which should be passed to the ADF component which will correctly place the button.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MichalKinas to move the close button to the right side in the header we have to add the close button in toolbar-actions, and for left position we are using the existing button only.

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you think you have to add new button in ACA? This is another thing that has to be maintained, is not documented properly, has naming issues and rule canClosePreview doesn't do what it's name suggests. This should be part of ADF component which should take a param with position of close button. This solution works in ACA only and overrides default component behaviour, I still think that the solution should be adding new param for ADF component and component itself should decide how to display the close button

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MichalKinas as per your suggestion I have removed the close button from toolbar-actions,now I am planning to add one more button in ADF viewer.component.html which will be show/hide as per the position set by customer in app.config.json which can be configuration from ACA or ADF.

@AnukritiGL AnukritiGL force-pushed the MNT-23433-preview-buttons-positions branch from 187bff8 to e0f6e1c Compare December 7, 2023 06:29
@@ -1132,6 +1132,7 @@
"downloadPromptDelay": 50,
"downloadPromptReminderDelay": 30,
"enableFileAutoDownload": true,
"fileAutoDownloadSizeThresholdInMB": 15
"fileAutoDownloadSizeThresholdInMB": 15,
"isCloseButtonOnLeft": false
Copy link
Contributor

@MichalKinas MichalKinas Dec 8, 2023

Choose a reason for hiding this comment

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

This comment is unfixed still, I also mentioned the same improvement in ADF PR

Done

@@ -13,6 +13,7 @@
[allowDownload]="false"
[allowFullScreen]="false"
[overlayMode]="true"
[allowGoBack]="'viewer.isCloseButtonOnLeft' | adfAppConfig: false"
Copy link
Contributor

Choose a reason for hiding this comment

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

allowGoBack should be unchanged here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

// todo: remove this when viewer supports extensions
.adf-viewer-toolbar > * > button:last-child {
.adf-viewer-toolbar > * > button:nth-last-child(3) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

[allowPrint]="false"
[showRightSidebar]="true"
[allowDownload]="false"
[allowFullScreen]="false"
[overlayMode]="true"
[closeButtonPosition]="'viewer.closeButtonPosition' | adfAppConfig: 'right'"
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you check if that works with left as a default value in ADF? And also if that works if the param is not set in the config file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I checked with default value as left as well working fine and also I try after removing the param from config file, it will come at right by default.

@AnukritiGL AnukritiGL force-pushed the MNT-23433-preview-buttons-positions branch from 94ea35d to a6c24e2 Compare December 28, 2023 12:16
@AnukritiGL AnukritiGL force-pushed the MNT-23433-preview-buttons-positions branch from a6c24e2 to 8545baf Compare January 2, 2024 12:42
@AnukritiGL AnukritiGL force-pushed the MNT-23433-preview-buttons-positions branch from 8545baf to db141b6 Compare January 3, 2024 06:12
Copy link

sonarcloud bot commented Jan 3, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@AnukritiGL AnukritiGL merged commit 6da7152 into develop Jan 3, 2024
26 checks passed
@AnukritiGL AnukritiGL deleted the MNT-23433-preview-buttons-positions branch January 3, 2024 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants