fundamental-styles icon indicating copy to clipboard operation
fundamental-styles copied to clipboard

feat(styles): notification in dialog

Open platon-rov opened this issue 2 years ago • 4 comments

Related Issue

Refers to https://github.com/SAP/fundamental-ngx/pull/8525

Description

Notifications in dialog update.

Screenshots

After:

image

platon-rov avatar Aug 18 '22 15:08 platon-rov

Deploy Preview for fundamental-styles ready!

Name Link
Latest commit ee17515b18652cf9c9eb747efad6bc496cfc2189
Latest deploy log https://app.netlify.com/sites/fundamental-styles/deploys/6332c259b3e95c0008c887d0
Deploy Preview https://deploy-preview-3818--fundamental-styles.netlify.app/
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

netlify[bot] avatar Aug 18 '22 15:08 netlify[bot]

  • There's no way to dismiss the dialog

Fixed.

  • On mobile the dialog should take the whole screen. Here are the guidelines: https://experience.sap.com/internal/fiori-design-web/dialog/#size-s-smartphone Also check the UX guidelines: https://experience.sap.com/internal/fiori-design-web/notification-center/

There is nothing about the mobile dialog, it's just about the notifications in the dialog. If needed, the dialog may be additionally configured.

platon-rov avatar Aug 19 '22 09:08 platon-rov

  • There's no way to dismiss the dialog

Fixed.

  • On mobile the dialog should take the whole screen. Here are the guidelines: https://experience.sap.com/internal/fiori-design-web/dialog/#size-s-smartphone Also check the UX guidelines: https://experience.sap.com/internal/fiori-design-web/notification-center/

There is nothing about the mobile dialog, it's just about the notifications in the dialog. If needed, the dialog may be additionally configured.

I provided these links as it specifies that the actions are in the footer: "On smartphones, a dialog can have one or two actions, which are located in the footer and right-aligned." This would mean that dismissing the notification dialog should be done by a Cancel button in the footer, not in the header (in my opinion) Screen Shot 2022-08-19 at 10 17 59 AM

InnaAtanasova avatar Aug 19 '22 14:08 InnaAtanasova

You should probably use the compact button and align it with the other dismiss buttons (that belong to the notifications) as this will be immediately reported as a bug by the designers.

Not a bug actually. Dialog has 1rem padding on the sides and notification has 0.5rem on the right side.

I provided these links as it specifies that the actions are in the footer:

Fixed.

platon-rov avatar Aug 22 '22 15:08 platon-rov

Screen Shot 2022-09-07 at 3 36 31 PM

Should it look like this in mobile?

InnaAtanasova avatar Sep 07 '22 19:09 InnaAtanasova

Should it look like this in mobile?

Nothing about notification itself, it comes from the dialog.

platon-rov avatar Sep 13 '22 15:09 platon-rov

Should it look like this in mobile?

Nothing about notification itself, it comes from the dialog.

it doesn't matter :) We shouldn't merge something that is broken or doesn't look ok. If it's coming from the dialog, either open a new issue that should be fixed asap and link it here, or fix it in a separate PR and link it here. But we can't merge this PR and just ignore the fact that in mobile the notifications look broken. @droshev what is your opinion?

InnaAtanasova avatar Sep 23 '22 13:09 InnaAtanasova

it doesn't matter :) We shouldn't merge something that is broken or doesn't look ok. If it's coming from the dialog, either open a new issue that should be fixed asap and link it here, or fix it in a separate PR and link it here. But we can't merge this PR and just ignore the fact that in mobile the notifications look broken. @droshev what is your opinion?

Opened a pr that fixes this issue

https://github.com/SAP/fundamental-styles/pull/3882

image

platon-rov avatar Sep 23 '22 15:09 platon-rov