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

Replace httpvueloader with vue3-sfc-loader #109

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

HeriLFIU
Copy link
Collaborator

@HeriLFIU HeriLFIU commented Oct 21, 2024

Closes #issue_number

Summary

Replaced httpvueloader with vue3-sfc-loader.

Mef-eline was trying to access and change the value of a prop within k-accordion-item through references, and props are read only. Because of this, the prop was switched for a data element since no component was using the prop functionality and the data element can be modified.

Mef-eline was trying to give the prop named value from k-input an integer value. This prop was typed and expected a string; because of this, the type specification was removed from the prop, and now it can accept any value.

Enabled the ability to disable k-input for text display purposes only.

Local Tests

To test the new additions all of the UI elements were used.

Note

k-info-panels are disabled (commented out), since they have not yet been incorporated with the changes.
Replace .kytos file extension with .vue.
Incorporate changes from drafts in other napps for vue3-sfc-loader compatibility.

@HeriLFIU HeriLFIU changed the title UI/vue3sfcloader modifications Replace httpvueloader with vue3-sfc-loader Oct 21, 2024
@HeriLFIU
Copy link
Collaborator Author

@viniarck I just finished testing all of the k-toolbars, and they are now functioning properly. There's a small error that I fixed, but it was a bit hard to detect. I believe that I got them all, but there is a change that I missed a couple, and unless explicitly used, it won't pop up. The error is that variables now need to be properly declared using either let or var; if not, the vue3-sfc-loader will throw an error. A lot of variables were missing that let/var.

@HeriLFIU
Copy link
Collaborator Author

But I went through all the buttons and inputs and it didn't show up again, so I think that the k-toolbars are in the clear.

Copy link
Member

@viniarck viniarck left a comment

Choose a reason for hiding this comment

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

@HeriLFIU, excellent how it's shaping up and having vue3-sfc-loader.

I've made some comments, trivial stuff though. Other than that, quoting that you pointed out:

The error is that variables now need to be properly declared using either let or var; if not, the vue3-sfc-loader will throw an error. A lot of variables were missing that let/var.

This part of non declared vars, have they been handled in other PRs or is this still future work that needs to be done? Let's keep that in mind before marking this PR ready and then we can try to land the changes. Thanks, Heriberto.

src/components/kytos/inputs/Input.vue Show resolved Hide resolved

getFile(url) {

return fetch(url).then(response => response.ok ? response.text() : Promise.reject(response));
Copy link
Member

Choose a reason for hiding this comment

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

Did you have the chance to try out and explore when the promise gets rejected?

Wondering how it'll be handled.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope, ill check it out to see what happens.

@HeriLFIU
Copy link
Collaborator Author

HeriLFIU commented Nov 1, 2024

@viniarck Yep, I am handling them in the individual PRs for each napp.

@HeriLFIU
Copy link
Collaborator Author

HeriLFIU commented Nov 1, 2024

@viniarck Also, the k-toolbars only had a few errors, which meant they were easy to fix, but the CSS for some of the info panels seems to break for some reason, and I'm trying to find out why.

@viniarck
Copy link
Member

viniarck commented Nov 4, 2024

@viniarck Yep, I am handling them in the individual PRs for each napp.

Excellent. The ones that could've got merged and wouldn't break anything I went ahead and merged them.

When this PR here is ready for review, I recommend you to also list in the description the other depends PR/issues that also need to land just so we make sure to merge them all together, to avoid potentially leaving something behind or potentially broken on master branch.

@viniarck
Copy link
Member

viniarck commented Nov 4, 2024

@viniarck Also, the k-toolbars only had a few errors, which meant they were easy to fix, but the CSS for some of the info panels seems to break for some reason, and I'm trying to find out why.

Right.

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.

2 participants