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(control-flow): migrate to the new control-flow via schematic command #49

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bougwal
Copy link
Collaborator

@bougwal bougwal commented May 9, 2024

Hey @falberthen I hope you are well!

So there is a lot going on on Angular these days and EcomDDD is behind with a lot of features...
I got snowed under with a lot of professional commitments the past few weeks.

I just ran the new Control Flow Syntax migration schematic command in the FE project to keep up with the upcoming changes. This is an automatic transformation by Angular utilities


I am using Colima to run the backend services - For some reason it gets hung and never ends - I assume it's my own env issue. I'll investigate it.


Thus, asking you to have a quick look before merging to check these syntactic sugar changes are fine, as I did not run it locally!

@bougwal bougwal changed the title feat(core): migrate to the new control-flow via schematic command feat(control-flow): migrate to the new control-flow via schematic command May 9, 2024
@bougwal bougwal requested a review from falberthen May 9, 2024 11:57
@falberthen
Copy link
Owner

Hey @falberthen I hope you are well!

So there is a lot going on on Angular these days and EcomDDD is behind with a lot of features... I got snowed under with a lot of professional commitments the past few weeks.

I just ran the new Control Flow Syntax migration schematic command in the FE project to keep up with the upcoming changes. This is an automatic transformation by Angular utilities

I am using Colima to run the backend services - For some reason it gets hung and never ends - I assume it's my own env issue. I'll investigate it.

Thus, asking you to have a quick look before merging to check these syntactic sugar changes are fine, as I did not run it locally!

Hey @bougwal, tks for submitting it. I'll test it this weekend and give you feedback. Regarding backend services, how you used to run then before? The way I recommend first running the docker compose. We keep in touch through dm if you need any help.

>
<span class="navbar-toggler-icon"></span>
</button>
@if (isLoggedIn) {
Copy link
Owner

Choose a reason for hiding this comment

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

We can safely remove this condition check.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch ! Looks redundant


It seems the comment was pending for 2 weeks I did not see it 🧐 - Sorry for the delay Felipe

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.

None yet

2 participants