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

Demo for strategy variants #4457

Merged
merged 11 commits into from
Aug 10, 2023
Merged

Demo for strategy variants #4457

merged 11 commits into from
Aug 10, 2023

Conversation

kwasniew
Copy link
Contributor

@kwasniew kwasniew commented Aug 9, 2023

About the changes

Screen.Recording.2023-08-09.at.15.12.45.mov
  • Variants in demo show new strategy variants instead of the feature variants
  • added cleanup code for accumulating strategies to get better perf and correctness. We remove all unconstrained strategies that were breaking demo for other users and we leave only last 30 constrained strategies.

Important files

Discussion points

@vercel
Copy link

vercel bot commented Aug 9, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
unleash-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 10, 2023 8:54am
unleash-monorepo-frontend ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 10, 2023 8:54am

Copy link
Member

@nunogois nunogois left a comment

Choose a reason for hiding this comment

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

Haven't tested it but at a glance this looks good 👍

I suggest going through the flow manually and also try to go back in some of the steps to see if it doesn't break.

Aligning with UX would also be a good idea just to be extra safe that this is what we intend.

]),
}
const tooGenericStrategies = results.filter(
(strategy: { constraints: Array<unknown>; segments: Array<unknown> }) =>
Copy link
Member

Choose a reason for hiding this comment

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

Don't we have the proper types for this instead of unknown?

Copy link
Contributor Author

@kwasniew kwasniew Aug 9, 2023

Choose a reason for hiding this comment

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

we don't care about those types so I left it as unknown. We care about the array length

},
},
]),
}
Copy link
Member

Choose a reason for hiding this comment

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

This means we no longer have one of red, green, blue on the demo app on a clean slate, right? Will it default to black? I honestly don't remember.

I'm OK with it, but it's something we should be aware of and maybe align with @EliseBrevald and @nicolaesocaciu

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't know the context of those so glad your brought it up. I can bring those back as fallback. It shouldn't change much for the strategy variants.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants