-
Notifications
You must be signed in to change notification settings - Fork 135
fix(core,platform): schematics, deps, migrations, assets & styles #8465
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
Conversation
2ab35e9
to
37d69eb
Compare
BREAKING CHANGES: * previously created styles and assets are outdated now. use `ng update` to fix them automatically. * after update run `ng add @fundamental-ngx/core` once again to automatically setup things in the new way.
✅ Deploy Preview for fundamental-ngx ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
Visit the preview URL for this PR (updated for commit 72639f3): https://fundamental-ngx-gh--pr8465-pr-fix-schematics-me-l3ntmedx.web.app (expires Sun, 31 Jul 2022 12:56:22 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
37d69eb
to
72639f3
Compare
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.
Awesome job!
|
||
function noticeAddSchematics(): Rule { | ||
return (_: Tree, context: SchematicContext) => { | ||
context.logger.info(`ℹ️ Now you have run ng-add schematics once again to setup things in the right way.`); |
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.
Can't we just run another migration here which will do what's needed?
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.
As I mentioned in the comment in the code below we can but we cannot pass options there. All the prompts will be asked again but $default
s won't be resolved properly.
overwrite: true | ||
}); | ||
} | ||
if ( |
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.
Can't we just move it to fundamental-ngx dependencies tree so it would automatically install needed version even if user simply updates the version manually in package.json of his/hers project?
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 you mean make styles & base-theming deps of the ngx/core - no we can't.
In the code below we set some assets & styles using relative paths
./node_modules/@sap-theming/theming-base-content/content/Base/baseLib/
In case we will have them as deps they may be placed under the specified path or under the following path
./node_modules/@fundamental-ngx/core/node_modules/@sap-theming/theming-base-content/content/Base/baseLib/
and it may change in future, see more about it in ticket #8350
}; | ||
} | ||
if (options.fonts) { | ||
additionalStyles.push('./node_modules/@fundamental-ngx/core/assets/fundamental-ngx-core-fonts.scss'); |
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 user's project uses less/css instead of scss? Maybe provide a simple css file which would 100% work?
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.
@@ -0,0 +1,6 @@ | |||
@font-face { |
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 think more correct way would be to move all css-related code to fundamental-styles theming. What do you think?
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 you mean fonts - agree, we could do this.
But common css libs/core/src/lib/assets/fundamental-ngx-core.scss
probably should be kept in ngx to have ability to add any code that might be not present in styles.
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.
Related Issue(s)
Closes #8350
Closes #8457
Refers to #8258
Description
TDB, Not done
Please check whether the PR fulfills the following requirements
PR Quality