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

Important notes (Dashboard Widget) #1297

Merged
merged 28 commits into from
Sep 20, 2022
Merged

Important notes (Dashboard Widget) #1297

merged 28 commits into from
Sep 20, 2022

Conversation

Schottkyc137
Copy link
Contributor

@Schottkyc137 Schottkyc137 commented May 28, 2022 •
edited
Loading

see issue: #1238

Visible/Frontend Changes

  • Add a dashboard widget that shows important notes
  • Make the notes configurable to a user

@github-actions
Copy link

Deployed to https://test-deployment-pr-1297.herokuapp.com/

@Schottkyc137 Schottkyc137 reopened this May 28, 2022
@github-actions
Copy link

Deployed to https://test-deployment-pr-1297.herokuapp.com/

@sleidig sleidig changed the title Important notes Important notes (Dashboard Widget) Jul 5, 2022
sleidig
sleidig reviewed Jul 5, 2022
View reviewed changes
Copy link
Member

@sleidig sleidig left a comment

Choose a reason for hiding this comment

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

Only review UI so far: I like how this looks and works overall 😃

Are the notes sorted by anything? Maybe by date makes sense?
Possibly could also indicate somehow whether it's "Urgent" or only "Needs Followup"

@sonarcloud
Copy link

sonarcloud bot commented Jul 6, 2022

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 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@Schottkyc137
Copy link
Contributor Author

All issues should be fixed now. I have also re-introduced the color as background since different notes can have a different color (red or orange)

TheSlimvReal
TheSlimvReal requested changes Aug 23, 2022
View reviewed changes
Copy link
Collaborator

@TheSlimvReal TheSlimvReal left a comment

Choose a reason for hiding this comment

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

Looks solid now @sleidig can you give a last UX review? I am still struggling a little with the very strong red color on all the warning notes.

src/app/child-dev-project/notes/dashboard-widgets/important-notes/important-notes.component.ts Outdated Show resolved Hide resolved
src/app/core/entity/mock-entity-mapper-service.ts Outdated Show resolved Hide resolved
@sleidig
Copy link
Member

sleidig commented Aug 25, 2022 •
edited
Loading

Very much like the widget overall. I do agree about the color being a bit overwhelming though. How about using the less heavy styling we have on the "record attendance" view, something like this:
image
(for reference, this is .row-view { border-left: 8px solid orange; }. Doesn't look perfectly clean yet without spacing, maybe we should try with an extra, empty but colored td instead)

@TheSlimvReal I think you did the technical code review? I didn't look closer at that yet.

@Schottkyc137
Copy link
Contributor Author

Very much like the widget overall. I do agree about the color being a bit overwhelming though. How about using the less heavy styling we have on the "record attendance" view, something like this: image (for reference, this is .row-view { border-left: 8px solid orange; }. Doesn't look perfectly clean yet without spacing, maybe we should try with an extra, empty but colored td instead)

@TheSlimvReal I think you did the technical code review? I didn't look closer at that yet.

I very much like the idea of having a more subtle hint instead of changing the whole background color. What do you think about the following?
Bildschirmfoto 2022-08-30 um 18 22 26

Though this is not more than a potential alternative; I don't have any strong opinions on either design.

@sonarcloud
Copy link

sonarcloud bot commented Sep 20, 2022

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.0% 0.0% Duplication

TheSlimvReal
TheSlimvReal approved these changes Sep 20, 2022
View reviewed changes
Copy link
Collaborator

@TheSlimvReal TheSlimvReal left a comment

Choose a reason for hiding this comment

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

Nice work, I like where we got with this and I think a lot of organisations can use this

@TheSlimvReal TheSlimvReal merged commit ba462de into master Sep 20, 2022
@TheSlimvReal TheSlimvReal deleted the important-notes branch September 20, 2022 14:03
@aam-digital-ci
Copy link
Collaborator

🎉 This PR is included in version 3.11.0-master.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@aam-digital-ci
Copy link
Collaborator

🎉 This PR is included in version 3.11.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@aam-digital-ci
Copy link
Collaborator

🎉 This PR is included in version 3.11.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

@sleidig sleidig sleidig left review comments

@TheSlimvReal TheSlimvReal TheSlimvReal approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Schottkyc137 @sleidig @aam-digital-ci @TheSlimvReal