Skip to content

Remove ng-deep#218

Open
PieterjanDeClippel wants to merge 4 commits into
masterfrom
remove-ng-deep
Open

Remove ng-deep#218
PieterjanDeClippel wants to merge 4 commits into
masterfrom
remove-ng-deep

Conversation

@PieterjanDeClippel

Copy link
Copy Markdown
Member

No description provided.

@nx-cloud

nx-cloud Bot commented Dec 28, 2024

Copy link
Copy Markdown

View your CI Pipeline Execution ↗ for commit 2cc161b.

Command Status Duration Result
nx affected --target=test --watch=false --paral... ✅ Succeeded 2m 29s View ↗
nx build --configuration=production ✅ Succeeded 1m 3s View ↗

☁️ Nx Cloud last updated this comment at 2024-12-29 00:37:58 UTC

@PieterjanDeClippel PieterjanDeClippel marked this pull request as draft December 29, 2024 14:06
@PieterjanDeClippel

Copy link
Copy Markdown
Member Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request aims to remove ng-deep from the codebase, which is a good step towards better encapsulation and maintainability. The changes involve removing ::ng-deep and :host ::ng-deep selectors from SCSS files and adding encapsulation: ViewEncapsulation.None to the corresponding components. Additionally, some minor HTML and SCSS adjustments were made. Overall, the changes seem reasonable, but let's take a closer look.

Summary of Findings

  • Potential style leakage with ViewEncapsulation.None: While removing ng-deep is beneficial, setting encapsulation: ViewEncapsulation.None can lead to style leakage if not carefully managed. It's important to ensure that component styles don't unintentionally affect other parts of the application.
  • Redundant ViewEncapsulation.None?: It's worth investigating whether all instances of ViewEncapsulation.None are truly necessary. In some cases, refactoring the CSS might be a better alternative to achieve the desired styling without disabling encapsulation.
  • Impact on Swiper Component: The removal of the entire SCSS block in swiper.component.scss might have unintended consequences on the component's styling. It's crucial to verify that the component still renders correctly and that all necessary styles are applied through other means.

Merge Readiness

The pull request is a step in the right direction by removing ng-deep. However, the introduction of ViewEncapsulation.None and the removal of styles in the Swiper component raise concerns that need to be addressed before merging. I recommend carefully reviewing the impact of these changes on the application's styling and considering alternative solutions where appropriate. I am unable to approve this pull request, and recommend that others review and approve this code before merging.

@PieterjanDeClippel PieterjanDeClippel marked this pull request as ready for review March 12, 2025 15:06
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.

1 participant