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

OSOE-476: Extract inline javascript block into file, and register for usage in Lombiq.HelpfulExtensions #114

Merged
merged 13 commits into from
Feb 18, 2023

Conversation

TheHydes
Copy link
Contributor

@TheHydes TheHydes commented Feb 16, 2023

@DemeSzabolcs
Copy link
Member

@TheHydes You can also commit the pnpm-lock.yaml file. It's also commited in other submodules like Chart Js: https://github.com/Lombiq/Orchard-Chart.js/blob/dev/Lombiq.ChartJs/pnpm-lock.yaml

@TheHydes
Copy link
Contributor Author

@TheHydes You can also commit the pnpm-lock.yaml file. It's also commited in other submodules like Chart Js: https://github.com/Lombiq/Orchard-Chart.js/blob/dev/Lombiq.ChartJs/pnpm-lock.yaml

Since its optional, i would rather leave it out at this point.

@DemeSzabolcs
Copy link
Member

@TheHydes You can also commit the pnpm-lock.yaml file. It's also commited in other submodules like Chart Js: https://github.com/Lombiq/Orchard-Chart.js/blob/dev/Lombiq.ChartJs/pnpm-lock.yaml

Since its optional, i would rather leave it out at this point.

Well for the project to run on GH it's not needed, but in the long run it will be annoying for the developers, since it will be generated every time:
image
image

And also as I linked it's commited in other submodules too.
So commit it please.

@DemeSzabolcs DemeSzabolcs merged commit c19e06b into dev Feb 18, 2023
Comment on lines +8 to +9
(links[i].hostname !== currentHostname &&
(!links[i].href.match(/^javascript:/i)))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This has been merged now, but there are superfluous parentheses here:

Suggested change
(links[i].hostname !== currentHostname &&
(!links[i].href.match(/^javascript:/i)))) {
links[i].hostname !== currentHostname &&
!links[i].href.match(/^javascript:/i)) {

window.addEventListener(
'load',
() => {
window.setTimeout(targetBlank, 100);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the timeout necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see it was in the original source.

The whole setTimeout call shouldn't be necessary, though.

Comment on lines +9 to +14
"nodejsExtensions": {
"scripts": {
"source": "Assets/Scripts",
"target": "wwwroot/scripts"
}
},
Copy link
Contributor

Choose a reason for hiding this comment

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

This whole configuration is superfluous, if you're willing to accept the default path wwwroot/js for JS files.

Comment on lines +15 to +22
"devDependencies": {
"eslint": "^8.25.0",
"eslint-config-airbnb-base": "^15.0.0",
"eslint-plugin-import": "^2.26.0",
"eslint-plugin-node": "^11.1.0",
"eslint-plugin-only-warn": "^1.0.3",
"eslint-plugin-promise": "^6.1.0"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

When used as part of OSOCE, this whole block should not be here.

@0liver
Copy link
Contributor

0liver commented Feb 20, 2023

I've created #116 to address the above and some more.

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.

3 participants