-
Notifications
You must be signed in to change notification settings - Fork 0
feat(Popup): add new custom element with dual-mode dialog and ribbon functionality #436
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
feat(Popup): add new custom element with dual-mode dialog and ribbon functionality #436
Conversation
Co-authored-by: timowestnosto <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot fix these
… remove extra stories Co-authored-by: timowestnosto <[email protected]>
All feedback has been addressed in commit [1fbfa87]: ✅ Storybook Changes:
✅ Architecture Refactoring:
The component maintains 100% functionality while following established architectural patterns. All tests pass with 98.5% coverage. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot fix these
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot fix these
…nto isPopupShown Co-authored-by: timowestnosto <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a new nosto-popup
custom element that displays popup content with dialog and ribbon slots, supporting segment-based activation and persistent closure state management.
- Implements a new Popup web component following established architectural patterns
- Adds comprehensive test coverage with 18 tests covering all functionality
- Extracts styles to separate module and refactors methods to top-level functions
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
test/utils/jsx.ts | Adds Popup component to JSX type definitions for testing |
test/components/Popup/Popup.spec.tsx | Comprehensive test suite covering all component functionality |
src/components/Popup/styles.ts | Extracted styles for the popup component |
src/components/Popup/Popup.ts | Main component implementation with dialog/ribbon slots and state management |
src/components/Popup/Popup.stories.tsx | Storybook stories demonstrating component usage |
* <span>Limited time!</span> | ||
|
||
* </div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The JSDoc example is incomplete. The ribbon slot example is missing the closing element and n-close button.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot fix this
All feedback addressed in commit [6a53239]: ✅ Code Organization:
✅ Architecture Improvements:
All tests pass with maintained 98.6% coverage and full functionality preserved. |
…e empty line Co-authored-by: timowestnosto <[email protected]>
All feedback addressed in commit [72ca515]: ✅ Code Cleanup:
✅ Enhanced Functionality:
✅ Quality Maintained:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot fix these
@timowestnosto Unfortunately I hit an unexpected error while processing your comment. I've automatically reported this to GitHub. You can ask me to try again later by mentioning me in a new comment. If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: Sorry for the inconvenience! |
@timowestnosto Unfortunately I hit an unexpected error while processing your comment. I've automatically reported this to GitHub. You can ask me to try again later by mentioning me in a new comment. If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: Sorry for the inconvenience! |
@timowestnosto Unfortunately I hit an unexpected error while processing your comment. I've automatically reported this to GitHub. You can ask me to try again later by mentioning me in a new comment. If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: Sorry for the inconvenience! |
bc35ab7
to
e5b2606
Compare
e5b2606
to
b869c7a
Compare
@copilot add Popup to readme file under "Store level templating" |
@copilot add Popup to readme file under "Store level templating" |
src/components/Popup/Popup.ts
Outdated
element.style.display = "none" | ||
} | ||
|
||
async function getPopupState(name: string, segment?: string): Promise<"open" | "ribbon" | "closed"> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PopupState type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
</dialog> | ||
<div class="ribbon ${mode === "open" ? "hidden" : ""}" part="ribbon"> | ||
<slot name="ribbon">Open</slot> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibilities for avoiding string templates?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using the html function here doesn't provide much additional value as none of the injections should be escaped
@@ -0,0 +1,40 @@ | |||
export const popupStyles = ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This type of css stying is hard to maintain. Should we instead use a regular css and import it in the popup.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I am planning to improve CSS resource handling in a follow up PR
Do we plan to promote this as an alternative to our Nosto's native popups? If so, do we have plans to handle triggers and rules? |
Yes, this PR provides the basic frame for implementing Popups via Nosto custom elements. Extra functionality can be agreed on later. |
styles.ts
file for better maintainabilityisPopupShown()
functionn-close
attributedisconnectedCallback
method for simplified lifecyclegetPopupState()
returning "open" | "ribbon" | "closed"n-ribbon
click handling for dual-mode operationgetPopupState()
setPopupState()
function for cleaner APIrenderShadowContent
withupdateShadowContent
for performancegetKey()
function🎉 Popup Component Implementation Complete
This PR introduces a new
nosto-popup
custom element that displays popup content with advanced features for conditional display, persistent state management, and dual-mode operation with optimized performance.✨ Key Features
🎭 Dual Display Modes
n-ribbon
to switch to ribbon mode🎯 Smart Targeting
segment
attribute🖱️ Interactive Controls
n-close
attribute will close the popup permanentlyn-ribbon
attribute will switch to ribbon mode🎨 Styling Flexibility
dialog
,ribbon
, andbackdrop
parts for external stylingdialog
and.ribbon
for clean CSS targeting::part()
selectors🔧 Usage
📊 Test Coverage
🏗️ Architecture
styles.ts
with shadow DOM parts and semantic selectorsgetPopupState()
function handles both segment checking and localStoragesetPopupState()
manages all state transitions ("open" | "ribbon" | "closed")close
,toRibbon
)getKey()
function for localStorage key generation🚨 Breaking Changes
name
attribute for proper state management"true"
to"closed"
for better semantic meaning🎨 Shadow DOM Parts & Semantic Selectors
dialog
: The modal dialog container (uses semantic<dialog>
element)ribbon
: The ribbon container in bottom-right corner (uses.ribbon
class selector)backdrop
: The dialog backdrop (usesdialog::backdrop
selector)🚀 Performance Optimizations
updateShadowContent()
only toggles visibility instead of re-rendering entire shadow DOMgetPopupState()
dialog
and.ribbon
for efficient DOM queries🧪 Testing Excellence
📚 Storybook Examples
name
attributen-ribbon
button for mode switching demonstration📸 Visual Preview
The final implementation provides a robust, well-documented popup component with dual-mode operation, performance-optimized DOM manipulation, enhanced click handling, semantic CSS architecture, comprehensive test coverage, complete Storybook examples, proper styling examples in JSDoc, and flexible styling options while following all established patterns.
Fixes #435
Original prompt
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.