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.

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(CNE-10708): add CNE Ticketing Widget #1705

Merged
merged 2 commits into from
Jan 15, 2024
Merged

Conversation

fedeava
Copy link
Contributor

@fedeava fedeava commented Dec 5, 2023

Add CNE Ticketing Widget

code to insert into CKeditor in copilot
<cne-ticketing-widget url="https://baseurl?loggedout=loggedouturlslug&loggedin=loggedinslug&privacy=true"></cne-ticketing-widget>

markdown
[#cneticketingwidget: https://baseurl?loggedout=loggedouturlslug&loggedin=loggedinslug&privacy=true]

embed
<cne-ticketing-widget-embed url="https://baseurl?loggedout=loggedouturlslug&loggedin=loggedinslug&privacy=true"></cne-ticketing-widget-embed>

@fedeava fedeava added the 🎉 Feature A new feature or request label Dec 5, 2023
url: string;
}> {
static vendorPrefix = "offset";
static type = "cne-ticketing-widget-embed";
Copy link
Contributor

@bachbui bachbui Dec 12, 2023

Choose a reason for hiding this comment

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

We've been going back and forth on naming. For sure I think we don't need widget in the annotation name. Perhaps
cne-event-registration-embed or cne-swoogo-embed ?

Copy link
Contributor Author

@fedeava fedeava Dec 13, 2023

Choose a reason for hiding this comment

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

@bachbui We prefer cne-event-registration-embed

code to insert into CKeditor in copilot
<cne-event-registration url="https://baseurl?loggedout=loggedouturlslug&loggedin=loggedinslug&privacy=true"></cne-event-registration>

markdown
[#cneeventregistration: https://baseurl?loggedout=loggedouturlslug&loggedin=loggedinslug&privacy=true]

embed
<cne-event-registration-embed url="https://baseurl?loggedout=loggedouturlslug&loggedin=loggedinslug&privacy=true"></cne-event-registration-embed>

Is that okay? I wait for a response before proceeding with the changes (file names, classes, tests, etc.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks sounds great to me

Copy link
Contributor Author

@fedeava fedeava Dec 15, 2023

Choose a reason for hiding this comment

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

@bachbui rename cne-ticketing-widget to cne-event-registration
Let me know if everything is correct.

},
})
);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a note - the purpose for this is only to support a conversion from this kind of contrived html snippet. If in the future users would have a different way of producing this kind of embed (for example, perhaps in a rich-text environment rather than inserting via an embed code, the user would have a form to enter in the loggedIn/loggedOut URLs and privacy flag) then we could remove this

@@ -214,6 +215,22 @@ export default function convertThirdPartyEmbeds(doc: Document) {
})
);
});
/**
* CNE Ticketing Widget
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we please update this comment to describe what this HTML structure is? IE, this is a custom HTML element used to describe an event registration block with a specific URL, with some specific metadata encoded in the URL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add a short description, thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated comment with a short description

Copy link
Contributor

@bachbui bachbui left a comment

Choose a reason for hiding this comment

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

Left some quick comments - mainly I would like to rename the annotation in offset-annotations to match our naming conventions used elsewhere. Secondly, it would be nice to add some comments in the html converter where we are converting a custom HTML element

@fedeava fedeava requested a review from bachbui December 19, 2023 08:21
@fedeava fedeava merged commit 0cc6554 into main Jan 15, 2024
3 checks passed
@fedeava fedeava deleted the feat/cne-ticketing-widget branch January 15, 2024 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎉 Feature A new feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants