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: move js code implemented directly on library.html (dynamo side) to librariejs #225

Conversation

Enzo707
Copy link
Contributor

@Enzo707 Enzo707 commented Apr 25, 2024

We have implemented javascript code, at some point in the past, related to library web component functionalities, directly into Dynamo. So now that we want to consume library as npm package, we need to move that functionalities keeping all library's responsibilities together one side in order to just install the package into Dynamo side.

@Enzo707 Enzo707 force-pushed the DYN-6866/migrate-js-code-from-dynamo-to-librariejs-component branch from e26df47 to d134c5d Compare April 25, 2024 23:28
@RobertGlobant20
Copy link
Contributor

@Enzo707 with this changes, could you add a GIF showing the expected behavior when running the guided tours in Dynamo?
image

@Enzo707
Copy link
Contributor Author

Enzo707 commented Apr 29, 2024

@Enzo707 with this changes, could you add a GIF showing the expected behavior when running the guided tours in Dynamo? image

@RobertGlobant20 I've tested and the guide works just fine

@mjkkirschner
Copy link
Member

looks like sonar cloud has found a few issues.

@Enzo707
Copy link
Contributor Author

Enzo707 commented Apr 29, 2024

looks like sonar cloud has found a few issues.

@mjkkirschner Yes, we didn't implement sonarcloud validations from the start so every time we change a file the scan throws a bunch of issues. The one I couldn't address is related to a function we expose to be used on Dynamo side so seems to be defined but not used. Can we just merge it anyway or how should we proceed?

index.html Show resolved Hide resolved
@QilongTang
Copy link
Contributor

Hi @Enzo707 Thanks for proposing this PR, is there a pairing PR on Dynamo side to remove these code?

index.html Show resolved Hide resolved
@Enzo707
Copy link
Contributor Author

Enzo707 commented Apr 29, 2024

Hi @Enzo707 Thanks for proposing this PR, is there a pairing PR on Dynamo side to remove these code?

Not yet, I wanted to bring this up first. If you agree I can created a PR on Dynamo repo removing this code.

@QilongTang
Copy link
Contributor

@Enzo707 Also notice you may want to bump up the version number for this change so we can re-publish

@Enzo707
Copy link
Contributor Author

Enzo707 commented Apr 29, 2024

@Enzo707 Also notice you may want to bump up the version number for this change so we can re-publish

Sure, I can do that. I have to rebase this branch also, as we already merged Roberto's PR related to library search.

@QilongTang
Copy link
Contributor

QilongTang commented Apr 29, 2024

@Enzo707 Also notice you may want to bump up the version number for this change so we can re-publish

Sure, I can do that. I have to rebase this branch also, as we already merged Roberto's PR related to library search.

Thanks, I am not sure about the rebase though. If you have not touched the same function, it may be fine

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

@QilongTang QilongTang merged commit 5bd2f15 into DynamoDS:master Apr 30, 2024
10 of 11 checks passed
Enzo707 pushed a commit to Enzo707/librarie.js that referenced this pull request May 16, 2024
QilongTang pushed a commit that referenced this pull request May 17, 2024
* Revert "Fixing DevTools Console Errors (#227)"

This reverts commit f5853c1.

* Revert "feat: move js code implemented directly on library.html (dynamo side) to librariejs (#225)"

This reverts commit 5bd2f15.

* remove resources on npm run production to not include them in published package

* feat: make file structure consistent than other web components

---------

Co-authored-by: enzo707 <[email protected]>
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.

5 participants