Skip to content
This repository has been archived by the owner on Jul 1, 2024. It is now read-only.

Fix [BUG] Icon copy notification text is not readable as the underlying elements are also visible. #9652

Closed
wants to merge 1 commit into from

Conversation

mrinmay7875
Copy link

Closes #9618

Fixes Issue

This PR fixes #9618

Changes proposed

  • Removed the unnecessary z index from navbar and now the Icon copy notification shows over the navbar as it should and text in the icon notification is readable properly.

Screenshots

Here is a screenshot after my fix:
image

@github-actions github-actions bot added the issue linked Pull Request has issue linked label Oct 28, 2023
@Kamaruddheen
Copy link
Member

Hey @mrinmay7875, I was thinking, what if we bumped up the z-index for the notification or alert instead of removing it from the navbar? Removing the navbar's z-index might potentially affect other components, and I thought this could be a more targeted solution. What do you reckon?

@mrinmay7875
Copy link
Author

@Kamaruddheen I did test it well but could not find any area where it is broken due to removing the z-index from navbar. Let's see what the maintainers say about this.

Copy link
Member

@eddiejaoude eddiejaoude left a comment

Choose a reason for hiding this comment

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

Thank you.

I agree with the other review, is it possible to increase the z-index of the alert notification?

@Kamaruddheen
Copy link
Member

It should be possible if we are generating a alert template from our end.

@Kamaruddheen
Copy link
Member

I could see alert.js file which is generating alerts based on success, errors, warning and info. If we increase the z-index of those class we should also get the same result.

@Kamaruddheen
Copy link
Member

Kamaruddheen commented Nov 1, 2023

This issue relates to the Notification component and its interaction with the IconCard component, not the Alert component!

After some investigation, I've pinpointed the actual bug. It turns out that z-40 is already defined in the Notification component, specifically on line no. 54

<div
aria-live="assertive"
className="pointer-events-none fixed z-40 inset-0 flex items-end px-4 py-6 sm:items-start sm:p-6"
>
<div className="flex w-full flex-col items-center space-y-4 sm:items-end">
<Transition
show={show}
as={Fragment}
enter="transform ease-out duration-300 transition"
enterFrom="translate-y-2 opacity-0 sm:translate-y-0 sm:translate-x-2"
enterTo="translate-y-0 opacity-100 sm:translate-x-0"
leave="transition ease-in duration-200"
leaveFrom="opacity-100"
leaveTo="opacity-0"
>
<div className="pointer-events-auto w-full max-w-sm overflow-hidden rounded-lg bg-white shadow-lg ring-1 ring-black ring-opacity-5">
<div className="p-4">
<div className="flex items-start">
<div className="flex-shrink-0">{iconComponent}</div>
<div className="ml-3 w-0 flex-1 pt-0.5">
<p className="text-sm font-medium text-primary-high">

I attempted to include the z-index within a <div> element of the Transition component, but it seems that the z-index is having an no impact due to the presence of position: static. I looked further, I couldn't identify any instances of position: static within the <div> elements.

@Kamaruddheen
Copy link
Member

The <main> element had the dark:z-10 class, which conflicted with the already defined z-40 class in the Notification component.

To visualize how these z-index values affect the layering,

biodrop_error.mp4

To resolve this matter, a straightforward solution would be to raise the z-index value in the <main> element from 10 to 40. The video displays the notification above the navbar in the layer hierarchy.

biodrop_zindex_solution.mp4

@mrinmay7875
Copy link
Author

I am busy with some personal stuff. @Kamaruddheen You can take this over if you want and create a PR with your changes.

@Kamaruddheen
Copy link
Member

Kamaruddheen commented Nov 2, 2023

Okay @mrinmay7875, I think I might be able to commit in this PR branch.

@SaraJaoude
Copy link
Member

@mrinmay7875 if you cannot work on an issue please unassign yourself. I will do this for you on this occasion.

@Kamaruddheen committing to someone else's PR would cause confusion and extra admin, so please raise a new PR. I will assign the issue to you.

As a result of the above conversation I am closing this PR.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue linked Pull Request has issue linked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Icon copy notification text is not readable as the underlying elements are also visible.
4 participants