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

Add dynamic brand configuration to hide sidebar illustrations for crazy domains #190

Conversation

arunshenoy99
Copy link
Member

@arunshenoy99 arunshenoy99 commented Mar 6, 2023

#189 needs to be merged before this.

Copy link
Member

@officiallygod officiallygod left a comment

Choose a reason for hiding this comment

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

Why don't we put IllustrationPanel specific logic in one place inside the same file?
Something like this?? As the config passed is not step-specific and instead brand specific.

Screenshot 2023-03-06 at 7 47 41 PM

@arunshenoy99
Copy link
Member Author

Why don't we put IllustrationPanel specific logic in one place inside the same file? Something like this?? As the config passed is not step-specific and instead brand specific.

Screenshot 2023-03-06 at 7 47 41 PM

Why don't we put IllustrationPanel specific logic in one place inside the same file? Something like this?? As the config passed is not step-specific and instead brand specific.

Screenshot 2023-03-06 at 7 47 41 PM

@officiallygod Good question, I thought of this initially, it makes sense to add the brandConfig check to IllustrationPanel just for this case where we are adding the Panel based on the config, but considering broader cases like conditionally determining the order and kind of components to be shown (or) changing the behavior of the sidebar as defined by the config which might arise eventually, it makes more sense to leave this logic in the step sidebar components. As an example, let's say Brand 1 does not require an IllustrationPanel that shows the image but might require another component or set of components in its place (or) Brand 2 simply needs a different structure that does not have any of the components used above rather has its own set of Components as defined in the brandConfig and many more cases like these are examples where taking this approach (even though tedious), saves us a lot of time long term :)

@officiallygod
Copy link
Member

Why don't we put IllustrationPanel specific logic in one place inside the same file? Something like this?? As the config passed is not step-specific and instead brand specific.
Screenshot 2023-03-06 at 7 47 41 PM

Why don't we put IllustrationPanel specific logic in one place inside the same file? Something like this?? As the config passed is not step-specific and instead brand specific.
Screenshot 2023-03-06 at 7 47 41 PM

@officiallygod Good question, I thought of this initially, it makes sense to add the brandConfig check to IllustrationPanel just for this case where we are adding the Panel based on the config, but considering broader cases like conditionally determining the order and kind of components to be shown (or) changing the behavior of the sidebar as defined by the config which might arise eventually, it makes more sense to leave this logic in the step sidebar components. As an example, let's say Brand 1 does not require an IllustrationPanel that shows the image but might require another component or set of components in its place (or) Brand 2 simply needs a different structure that does not have any of the components used above rather has its own set of Components as defined in the brandConfig and many more cases like these are examples where taking this approach (even though tedious), saves us a lot of time long term :)

@arunshenoy99 I did think that but then I also thought that it won't be that good if we are sending brand-specific logic per step ( i.e around 15 steps ) of logic for conditional rendering from the backend making even the store very heavy.
But Sureee else everything looks goood.

@arunshenoy99 arunshenoy99 merged commit 2fbf1a8 into add/PRESS2-662-crazydomains-branding Mar 8, 2023
@arunshenoy99 arunshenoy99 deleted the enhance/crazydomains-hide-sidebar-illustrations branch March 8, 2023 08:11
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.

2 participants