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

refactor: remove empty harness e2e stylesheet #25304

Merged
merged 1 commit into from
Jul 19, 2022

Conversation

alkavats1
Copy link
Contributor

@alkavats1 alkavats1 commented Jul 19, 2022

No description provided.

@alkavats1 alkavats1 force-pushed the codeSmells-removed branch 2 times, most recently from c10ff75 to abbdbf7 Compare July 19, 2022 09:41
@andrewseguin
Copy link
Contributor

The commit message and description is not helpful. Please modify it to describe this change

@alkavats1 alkavats1 force-pushed the codeSmells-removed branch from abbdbf7 to b095614 Compare July 19, 2022 13:40
@alkavats1
Copy link
Contributor Author

I've modified the commit message. Please review it

@andrewseguin
Copy link
Contributor

The commit message refers to "code smells" - this is vague and still isn't helpful. I am expecting something more like "refactor: remove empty harness e2e stylesheet".

Also, your change is adding an unnecessary line to the mdc-migration app.component.ts file.

Can you help me understand more about why you are making this change? Is this practice contributing a pull request?

@alkavats1 alkavats1 force-pushed the codeSmells-removed branch from b095614 to 0f7fb0d Compare July 19, 2022 13:44
removed the empty files to remove the code smells from the code
@alkavats1 alkavats1 force-pushed the codeSmells-removed branch from 0f7fb0d to b179ef7 Compare July 19, 2022 13:47
@alkavats1
Copy link
Contributor Author

@andrewseguin Is it fine now?

@andrewseguin andrewseguin changed the title refactor: refactored the code and removed the code smells refactor: remove empty harness e2e stylesheet Jul 19, 2022
@andrewseguin andrewseguin added the target: patch This PR is targeted for the next patch release label Jul 19, 2022
@andrewseguin andrewseguin added the action: merge The PR is ready for merge by the caretaker label Jul 19, 2022
@andrewseguin andrewseguin merged commit 2905638 into angular:main Jul 19, 2022
andrewseguin pushed a commit that referenced this pull request Jul 19, 2022
removed the empty files to remove the code smells from the code

(cherry picked from commit 2905638)
andrewseguin pushed a commit that referenced this pull request Jul 19, 2022
removed the empty files to remove the code smells from the code

(cherry picked from commit 2905638)
@devversion
Copy link
Member

FYI that this was a default CLI app generated and it was meant to keep the same boilerplate. I'm good with this change though as I don't see any issue here. Still wanted to mention that this is intentional though.

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Aug 19, 2022
@andrewseguin andrewseguin self-assigned this Jun 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants