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

Remove Telerik #75

Open
wants to merge 3 commits into
base: development
Choose a base branch
from
Open

Conversation

erw13n
Copy link

@erw13n erw13n commented Sep 14, 2023

Description of PR...

Remove Telerik dependency and replace Treeview with Asp.Net Treeview

Please mark which issue is solved

Close #59

@valadas
Copy link
Member

valadas commented Sep 14, 2023

Thanks so much, this is awesome... I'll review properly shortly, I may do a followup PR to not entirely rely on cdn which would not work on some intranets, but from a quick look, this appears to be a great solution here.

Copy link
Member

@valadas valadas left a comment

Choose a reason for hiding this comment

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

Ok, so I took the time to test it and review properly. It worked fine for me but I have some concerns noted in this review.

Comment on lines -12 to -20
<asp:View ID="vShowCategoryTypeList" runat="server">
<div class="categoryList">
<dnn:DnnListBox runat="server" ID="listCategories" CssClass="categoryListControl" OnItemDataBound="listCategories_ItemDataBound">
<itemtemplate>
<asp:CheckBox ID="chkCategory" runat="server" Text='<%# Eval("FaqCategoryName") %>' OnCheckedChanged="chkCategory_CheckedChanged" AutoPostBack="true" />
</itemtemplate>
</dnn:DnnListBox>
</div>
</asp:View>
Copy link
Member

Choose a reason for hiding this comment

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

This removes a feature from the module

Comment on lines +102 to +106
if (this.Page.ClientScript.IsClientScriptBlockRegistered("pikaday.min.js") == false)
{
this.Page.ClientScript.RegisterClientScriptBlock(this.GetType(), "Moment.js", "<script language=javascript src=\"https://cdnjs.cloudflare.com/ajax/libs/moment.js/2.29.1/moment.min.js\"></script>");
this.Page.ClientScript.RegisterClientScriptBlock(this.GetType(), "AjaxFaq.js", "<script language=javascript src=\"https://cdn.jsdelivr.net/npm/pikaday/pikaday.min.js\"></script>");
}
Copy link
Member

Choose a reason for hiding this comment

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

This relies exclusivelly on cdns which could not work in some intranets with external internet access disabled

Copy link
Contributor

Choose a reason for hiding this comment

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

moment is deprecated. It is a pretty easy swap for dayjs. I am happy to help with that if desired. I can also help with converting this away from CDN. Since this is not a frontend project, I guess we would use local versions of the libraries (like we do in DNN)?

Comment on lines -404 to -415
if (noCat)
categories.Add(emptyCategory);
foreach (CategoryInfo cat in cats)
{
categories.Add(cat);
}
listCategories.DataSource = categories;
listCategories.DataBind();
mvShowCategoryType.SetActiveView(vShowCategoryTypeList);
pnlShowCategoryTypeDropdown.Visible = false;
break;

Copy link
Member

Choose a reason for hiding this comment

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

This removes a feature from the module

Comment on lines -485 to -514
//Filter on the checked items
foreach (RadListBoxItem item in listCategories.Items)
{

//Get the checkbox in the Control
CheckBox chkCategory = (CheckBox)(item.FindControl("chkCategory"));

//If checked the faq module is being filtered on one or more category's
if (chkCategory.Checked)
{

//Set Checked Flag
noneChecked = false;

//Get the filtered category
string checkedCategoryName = chkCategory.Text;

//Get the elements that match the catagory
var matchedCat = (from c in cats where c.FaqCategoryId == fData.CategoryId select c).SingleOrDefault();
categoryName = (matchedCat != null ? matchedCat.FaqCategoryName : "");

if ((categoryName == checkedCategoryName) ||
(fData.CategoryId == null && checkedCategoryName == Localization.GetString("EmptyCategory", LocalResourceFile)))
{
match = true;
break;
}
}
}
break;
Copy link
Member

Choose a reason for hiding this comment

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

This removes a module feature


protected void treeCategories_HandleDrop(object sender, RadTreeNodeDragDropEventArgs e)
Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly we loose the ability to order categories by removing this right ?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it remove the ability for Drag and drop, because asp.net treeview doesn't support it.

@@ -20,7 +20,6 @@
<dnn:Label ID="lblShowCategoryType" ControlName="rblShowCategoryType" runat="server">
</dnn:Label>
<asp:RadioButtonList ID="rblShowCategoryType" runat="server" CssClass="dnnFormRadioButtons">
<asp:ListItem Value="0" ResourceKey="ShowCategoryTypeList">List with checkboxes</asp:ListItem>
Copy link
Member

Choose a reason for hiding this comment

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

This removes support for "List with checkboxes"

* Pikaday
* Copyright � 2014 David Bushell | BSD & MIT license | https://dbushell.com/
*/
.pika-single{z-index:9999;display:block;position:relative;color:#333;background:#fff;border:1px solid #ccc;border-bottom-color:#bbb;font-family:"Helvetica Neue",Helvetica,Arial,sans-serif}.pika-single:after,.pika-single:before{content:" ";display:table}.pika-single:after{clear:both}.pika-single.is-hidden{display:none}.pika-single.is-bound{position:absolute;box-shadow:0 5px 15px -5px rgba(0,0,0,.5)}.pika-lendar{float:left;width:240px;margin:8px}.pika-title{position:relative;text-align:center}.pika-label{display:inline-block;position:relative;z-index:9999;overflow:hidden;margin:0;padding:5px 3px;font-size:14px;line-height:20px;font-weight:700;background-color:#fff}.pika-title select{cursor:pointer;position:absolute;z-index:9998;margin:0;left:0;top:5px;opacity:0}.pika-next,.pika-prev{display:block;cursor:pointer;position:relative;outline:0;border:0;padding:0;width:20px;height:30px;text-indent:20px;white-space:nowrap;overflow:hidden;background-color:transparent;background-position:center center;background-repeat:no-repeat;background-size:75% 75%;opacity:.5}.pika-next:hover,.pika-prev:hover{opacity:1}.is-rtl .pika-next,.pika-prev{float:left;background-image:url(data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABQAAAAeCAYAAAAsEj5rAAAAUklEQVR42u3VMQoAIBADQf8Pgj+OD9hG2CtONJB2ymQkKe0HbwAP0xucDiQWARITIDEBEnMgMQ8S8+AqBIl6kKgHiXqQqAeJepBo/z38J/U0uAHlaBkBl9I4GwAAAABJRU5ErkJggg==)}.is-rtl .pika-prev,.pika-next{float:right;background-image:url(data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABQAAAAeCAYAAAAsEj5rAAAAU0lEQVR42u3VOwoAMAgE0dwfAnNjU26bYkBCFGwfiL9VVWoO+BJ4Gf3gtsEKKoFBNTCoCAYVwaAiGNQGMUHMkjGbgjk2mIONuXo0nC8XnCf1JXgArVIZAQh5TKYAAAAASUVORK5CYII=)}.pika-next.is-disabled,.pika-prev.is-disabled{cursor:default;opacity:.2}.pika-select{display:inline-block}.pika-table{width:100%;border-collapse:collapse;border-spacing:0;border:0}.pika-table td,.pika-table th{width:14.285714285714286%;padding:0}.pika-table th{color:#999;font-size:12px;line-height:25px;font-weight:700;text-align:center}.pika-button{cursor:pointer;display:block;box-sizing:border-box;-moz-box-sizing:border-box;outline:0;border:0;margin:0;width:100%;padding:5px;color:#666;font-size:12px;line-height:15px;text-align:right;background:#f5f5f5;height:initial}.pika-week{font-size:11px;color:#999}.is-today .pika-button{color:#3af;font-weight:700}.has-event .pika-button,.is-selected .pika-button{color:#fff;font-weight:700;background:#3af;box-shadow:inset 0 1px 3px #178fe5;border-radius:3px}.has-event .pika-button{background:#005da9;box-shadow:inset 0 1px 3px #0076c9}.is-disabled .pika-button,.is-inrange .pika-button{background:#d5e9f7}.is-startrange .pika-button{color:#fff;background:#6cb31d;box-shadow:none;border-radius:3px}.is-endrange .pika-button{color:#fff;background:#3af;box-shadow:none;border-radius:3px}.is-disabled .pika-button{pointer-events:none;cursor:default;color:#999;opacity:.3}.is-outside-current-month .pika-button{color:#999;opacity:.3}.is-selection-disabled{pointer-events:none;cursor:default}.pika-button:hover,.pika-row.pick-whole-week:hover .pika-button{color:#fff;background:#ff8000;box-shadow:none;border-radius:3px}.pika-table abbr{border-bottom:none;cursor:help}
Copy link
Member

Choose a reason for hiding this comment

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

I am not a fan of just embeding minified 3rd party css int he module.css, makes it really hard to read and maintain

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. However, this is pikaday (we have this same problem in DNN).

Comment on lines +102 to +106
if (this.Page.ClientScript.IsClientScriptBlockRegistered("pikaday.min.js") == false)
{
this.Page.ClientScript.RegisterClientScriptBlock(this.GetType(), "Moment.js", "<script language=javascript src=\"https://cdnjs.cloudflare.com/ajax/libs/moment.js/2.29.1/moment.min.js\"></script>");
this.Page.ClientScript.RegisterClientScriptBlock(this.GetType(), "AjaxFaq.js", "<script language=javascript src=\"https://cdn.jsdelivr.net/npm/pikaday/pikaday.min.js\"></script>");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

moment is deprecated. It is a pretty easy swap for dayjs. I am happy to help with that if desired. I can also help with converting this away from CDN. Since this is not a frontend project, I guess we would use local versions of the libraries (like we do in DNN)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove Telerik
3 participants