Skip to content

Conversation

@Mudaafi
Copy link
Contributor

@Mudaafi Mudaafi commented Mar 1, 2024

This breaks the current interface of useRequestConfigs so there's a lot of refactoring involved.

Since we're not planning to expose useRequestConfigs to the consumer, we might want to hide/delete the storybook stories pertaining to it.

@linear
Copy link

linear bot commented Mar 1, 2024

CSL-3297 [MVP] Surface a generic `setRequestConfigs` function to ease the setting to the URL

Business Outcome:

  • As a Maintainer I want a generic function that I can use to update the state --> url so that I can reuse the same logic rather than deduplicating code

Note:

  • This is an interface change :sad_parrot: so there's bee alot of refactoring :sadblob:

Definition of done:

  • Update useRequestConfigs to return an object rather than just the config object
  • Update all instances where useRequestConfig is used
  • Update tests so they don't break
  • Update Documentation (if any) to include this

@Mudaafi Mudaafi requested a review from a team March 1, 2024 14:36
Copy link
Contributor

@HHHindawy HHHindawy left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for working on this!
one tiny comment 🤏


it('Should return any empty object', async () => {
const { result } = renderHookServerSideWithCioPlp(() => useRequestConfigs(), {
it('Should return any empty object and a setter', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

any -> an

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice eyes

@Mudaafi Mudaafi merged commit f2710f2 into main Mar 1, 2024
@Mudaafi Mudaafi deleted the csl-3297-mvp-surface-a-generic-setrequestconfigs-function-to-ease-the branch March 1, 2024 16:30
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.

3 participants