-
Notifications
You must be signed in to change notification settings - Fork 20
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
base: development
Are you sure you want to change the base?
Remove Telerik #75
Conversation
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. |
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.
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.
<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> |
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 removes a feature from the module
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>"); | ||
} |
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 relies exclusivelly on cdns which could not work in some intranets with external internet access disabled
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.
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)?
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; | ||
|
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 removes a feature from the module
//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; |
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 removes a module feature
|
||
protected void treeCategories_HandleDrop(object sender, RadTreeNodeDragDropEventArgs e) |
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.
If I understand correctly we loose the ability to order categories by removing this right ?
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, 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> |
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 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} |
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.
I am not a fan of just embeding minified 3rd party css int he module.css, makes it really hard to read and maintain
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.
Agreed. However, this is pikaday (we have this same problem in DNN).
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>"); | ||
} |
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.
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)?
Description of PR...
Remove Telerik dependency and replace Treeview with Asp.Net Treeview
Please mark which issue is solved
Close #59