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.

Sign up for GitHub

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

Jump to bottom

fix[dynamic-renderer]: ENG-8440 Button links cannot be used in Angular Gen 2 SDK #3937

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

fix[dynamic-renderer]: ENG-8440 Button links cannot be used in Angular Gen 2 SDK #3937

wants to merge 9 commits into from

Conversation

yash-builder
Copy link
Contributor

Description

This PR replaces the switch-case logic with a Map for dynamically selecting components in dynamic-renderer. The previous approach caused an assertion error when users typed.

Loom
https://www.loom.com/share/1229e7974db44743921df588ca1bd1bb?sid=ea3ccc9c-41de-4efe-b499-f61ca9244cbd

@yash-builder yash-builder self-assigned this Feb 27, 2025
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Feb 27, 2025 •
edited
Loading

🦋 Changeset detected

Latest commit: a922232

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@builder.io/sdk-angular Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@yash-builder yash-builder requested review from a team and sidmohanty11 and removed request for a team February 27, 2025 02:52
@nx-cloud Nx Cloud
Copy link

nx-cloud bot commented Feb 27, 2025 •
edited
Loading

View your CI Pipeline Execution ↗ for commit a922232.

Command Status Duration Result
nx test @e2e/qwik-city ✅ Succeeded 7m 39s View ↗
nx test @e2e/nextjs-sdk-next-app ✅ Succeeded 7m 33s View ↗
nx test @e2e/nuxt ✅ Succeeded 7m 12s View ↗
nx test @e2e/angular-16-ssr ✅ Succeeded 6m 33s View ↗
nx test @e2e/angular-19-ssr ✅ Succeeded 6m 12s View ↗
nx test @e2e/angular-16 ✅ Succeeded 6m 27s View ↗
nx test @e2e/react-sdk-next-pages ✅ Succeeded 5m 56s View ↗
nx test @e2e/gen1-react ✅ Succeeded 5m 53s View ↗
Additional runs (36) ✅ Succeeded ... View ↗

☁️ Nx Cloud last updated this comment at 2025-03-03 15:48:59 UTC

@yash-builder yash-builder added the bug Something isn't working label Feb 27, 2025
@shubham-builder
Copy link
Contributor

LGTM... need to consider the case for dynamic-elements for unknown. PR - #3892

samijaber
samijaber reviewed Feb 27, 2025
View reviewed changes
Copy link
Contributor

@samijaber samijaber left a comment

Choose a reason for hiding this comment

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

Can we add an integration test for this feature? It would be a visual editing test where the user is updating the value of the link

feat(dynamic button test): Added a test case to tackle assertion erro…
bdb5aa1 
…r when adding link to the button. Implemented typing simulation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sidmohanty11 sidmohanty11 Awaiting requested review from sidmohanty11

@samijaber samijaber Awaiting requested review from samijaber

At least 1 approving review is required to merge this pull request.

Assignees

@yash-builder yash-builder

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yash-builder @shubham-builder @samijaber @sidmohanty11