-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Blazor Web Component Blazor POC #15647
Conversation
- Options Editor and ModalDialog in Blazor - Using blazor in PredefinedList
This pull request has merge conflicts. Please resolve those before requesting a review. |
@sebastienros Here is one that is interesting to review. Looks like an interesting way to use Blazor. Need to analyze if it's better over an actual Vue app. |
Just comparing the amount of code necessary for this with the Vue app. Here we have a 91 lines of code to achieve this vs all this code for Blazor to create the same thing. Blazor will require a lot more code management. |
personally i would favor vanilla javascript and .net code over vue.js even if ir means more lines of code. |
What is .NET code these days? Typescript with Vue.js or anything else is C# syntax like too ... I need to take a deeper look later on and review this PR more. |
@@ -0,0 +1,10 @@ | |||
export function optionsChanged(element, jsonValue, defaultValue) { |
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.
Could this be renamed to .mjs so that we make a distinction between plain javascript and modules?
Also, need to understand why we need this javascript if we are using Blazor.
It throws an event to Blazor basically but why?
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.
To trigger the event to DOM, so it can be handled from any handler that can listen to such events
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.
Yeah, I understand that part. Can't these events be handled by Blazor? Seems like it can by doing a quick Google search.
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.
Something like <input @onchange=SetMessage
>
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 file using @export
suggests that it will be treated as an ECMAScript module.
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.
And that probably means that we would need to convert every small scripts to ES6 modules.
src/OrchardCore.Components/OrchardCore.Common.Components/OptionEditor.razor
Show resolved
Hide resolved
</div> | ||
<script at="Foot"> |
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.
We really need to have a bundler to start having script type="module" and get rid of these template scripts.
if (JS is not null) | ||
{ | ||
JSModule = await JS.InvokeAsync<IJSObjectReference>("import", | ||
$"./_content/{ns}/{cmp}.razor.js"); |
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.
Here the ES6 module are imported based on their name. This should be renamed to .mjs to make a distinction or support it at least.
# Conflicts: # OrchardCore.sln # src/OrchardCore.Modules/OrchardCore.Admin/Startup.cs
This pull request has merge conflicts. Please resolve those before requesting a review. |
It seems that this pull request didn't really move for quite a while. Is this something you'd like to revisit any time soon or should we close? Please comment if you'd like to pick it up. |
Closing this pull request because it has been stale for very long. If you think this is still relevant, feel free to reopen it. |
@ns8482e Just creating a draft PR here as it is easier for me to review.
Related discussion: #15653