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

[ACS-5166] Changes to Digital workspace new UI for better experience #3264

Closed
wants to merge 11 commits into from

Conversation

arohilaGL
Copy link
Contributor

@arohilaGL arohilaGL commented Jun 8, 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)

https://alfresco.atlassian.net/browse/ACS-5166

What is the new behaviour?

Design changes to new UI layout

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: ...

Alfresco/alfresco-ng2-components#8640, must be merged before merging this PR.

Other information:

@DenysVuika
Copy link
Contributor

@arohilaGL please provide jira link in the PR description, and also screenshots of the end results. Based on my tests, the icons are broken in this PR

@DenysVuika
Copy link
Contributor

@arohilaGL if this PR depends on other PRs, you should state this in the description. This PR relies on ADF changes

Copy link
Contributor

Choose a reason for hiding this comment

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

we already have material icons font, you are adding a second font there that is "Material Symbols", not "Material Icons"

app/project.json Outdated
@@ -95,6 +95,7 @@
],
"styles": [
"app/src/assets/fonts/material-icons/material-icons.css",
"app/src/assets/fonts/material-icons/material-icons-outlined.css",
Copy link
Contributor

Choose a reason for hiding this comment

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

you are loading additional font on top of existing font in line 97

@DenysVuika
Copy link
Contributor

This PR is using wrong library, should be using Material Icons instead
Screenshot 2023-06-19 at 16 28 43

@@ -16,6 +16,7 @@
<button
[id]="actionRef.id"
[color]="data?.color || color"
[ngClass]="{'secondary-button-color': !data?.color}"
Copy link
Contributor

Choose a reason for hiding this comment

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

you set the "primary" color in the app config, but this is not reflected as you just force a static background

display: flex;
}
.secondary-button-color {
background: var(--theme-app-toolbar-button-background-color);
Copy link
Contributor

Choose a reason for hiding this comment

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

how does the "primary" color in the app config matter here?

@DenysVuika
Copy link
Contributor

reworking the PR (#3287) so closing this one

@DenysVuika DenysVuika closed this Jun 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants