-
Notifications
You must be signed in to change notification settings - Fork 680
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
TypeScript version of pds-toolkit-react #9177
Conversation
Outstanding discussion blocking merge: From @IngridKwok
From @mel-miller
UPDATE:
If you prefer the colors on the right, let me know and I'll update this PR |
⚡ Deployed with Pantheon Decoupled This build was successfully deployed with Pantheon. You can track the build logs here. 👀 Preview: https://pr-9177-documentation.appa.pantheon.site |
Blocked: New error after selecting a category filter, introduced by syncing the main branch. The error is not observed on the preview environment, it is only observed locally Mel has a likely fix and will review with Gus |
I reviewed colors with Ingrid in a DM and the alternative suggested above is preferred |
Remove `updateQueryStrings()` call — this had been moved to the template file.
⚡ Deployed with Pantheon Decoupled This build was successfully deployed with Pantheon. You can track the build logs here. 👀 Preview: https://pr-9177-documentation.appa.pantheon.site |
⚡ Deployed with Pantheon Decoupled This build was successfully deployed with Pantheon. You can track the build logs here. 👀 Preview: https://pr-9177-documentation.appa.pantheon.site |
1 similar comment
⚡ Deployed with Pantheon Decoupled This build was successfully deployed with Pantheon. You can track the build logs here. 👀 Preview: https://pr-9177-documentation.appa.pantheon.site |
that seems to have fixed the error, but now when i click to filter by category, the popup disappears before i can interact with it. if i click it again, it stays open Screen.Recording.2024-08-15.at.1.36.13.PM.movreassigning to @gfbollingerHakuna per slack discussion with mel |
Hi @rachelwhitton, I couldn't fully reproduce the issue, but I assume the problem occurs when you open the Popover before the data is fully loaded. This might cause the parent to re-render and close the Popover. I added a small snippet to disable the Popover button in the meantime. Let me know what you think and if it works for you. Thanks! |
⚡ Deployed with Pantheon Decoupled This build was successfully deployed with Pantheon. You can track the build logs here. 👀 Preview: https://pr-9177-documentation.appa.pantheon.site |
⚡ Deployed with Pantheon Decoupled This build was successfully deployed with Pantheon. You can track the build logs here. 👀 Preview: https://pr-9177-documentation.appa.pantheon.site |
I'm still see the same issue locally, @jazzsequence are you able to reproduce what I'm seeing? |
I noticed if I interact with the cookie banner before the filter things work fine. But if I do not interact with the cookie banner first, that's when I see the issue Screen.Recording.2024-08-22.at.10.51.18.AM.mov |
I can reproduce the same behavior that @rachelwhitton notes. Specifically:
After this, subsequent attempts to use the filter by category dropdown work fine. Alternately:
This seems like a layer/z-index thing maybe? It seems to only happen when the cookies banner exists (because it's on top). If you interact with the dropdown while the cookies banner is visible, the dropdown cancels the cookies banner out while closing both simultaneously. Tested in codespaces on this branch. https://www.loom.com/share/4dad8eff76194a028ab696ce6353d1d9?sid=8771748d-8492-4988-adf8-bff10c1f127a |
@gfbollingerHakuna can you take a look at the recent comments and let us know? cc @mel-miller |
Hi @rachelwhitton. First of all, I apologize for the delayed response. I wasn’t receiving GitHub notifications due to a trusted domain issue (I didn't have my pantheon.io email linked with my github account), so I completely missed your comments. Are you still experiencing the issue? I ran a couple of checks in my local environment and on the staging link provided via GitHub, but I can’t reproduce it now, even though I was able to earlier. Here’s my Loom: https://www.loom.com/share/fd9b15c7af9c4094a7d49c17d1ada29b. I noticed that the cookies banner is coming from an external script provided by OneTrust. I’m thinking the issue might have been on their end regarding z-index styles, and maybe they’ve fixed it? Let me know your thoughts and if you’re still encountering the bug. I’ll keep an eye on it. Thanks! |
@gfbollingerHakuna yes I'm still experiencing this issue when running the docs site locally on this branch. I'm now realizing I also see the issue on the main branch when working locally. I thought I tested that originally but maybe not. @jazzsequence can you check to see if you also see this issue on the |
I am still able to reproduce the issue on |
I will note as an addendum to ☝️ that I had to actually delete the stored cookies I had in order to reproduce. If you've saved the cookies, the banner doesn't appear, which is the whole issue. |
Replaces #9113
This PR:
Summary
We have recently updated the PDS toolkit to be compatible with TypeScript. This PR updates to the lastest version and tests that the bundle is working correctly — which it is!
New tag colors
This update also introduces a few new changes to the Tag component including truncation for long tag names, an expanded palette of colors (20 total now), and the tags each now have a title attribute included.
Left: proposed tag colors (preview link) ------- Right: current tag colors (live link)