Skip to content

Conversation

KIvanow
Copy link
Contributor

@KIvanow KIvanow commented May 16, 2025

No description provided.

@KIvanow KIvanow requested a review from ArtemHoruzhenko May 19, 2025 13:57
style={{ marginTop: '12px' }}
>
{parse(consent.description)}
{getText()}
Copy link
Collaborator

Choose a reason for hiding this comment

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

a more React way would be to use jsx instead of a function call

Suggested change
{getText()}
<ItemDescription description={consent.description && parse(consent.description)} withLink={linkToPrivacyPolicy} />

and can define the component outside ConsentOption

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be a more React way. However I am not sure creating a component that will be used only in this case and prop drill it with the data from the parent one is optimal either.
Is there another benefit of not having a function call, other than sticking to jsx?

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 to @KrumTy suggestion

Honestly can't figure out a major benefit, but the produced code looks better and simpler, and maybe the most important part - generally sets the tone to not use it, especially on places where some things can be reused or simplified 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the produced code looks better and simpler, and maybe the most important part - generally sets the tone to not use it, especially on places where some things can be reused or simplified

I am not sure how the code looks better and simpler, but also sets the tone of not using it. This seems like a contradiction.
Anyway, I replaced a function call with 2 separate files and a folder. Even has an index.tsx to re-export it to follow all React patterns.

@ArtemHoruzhenko
Copy link
Collaborator

@KIvanow could you please look on this. https://github.com/RedisInsight/RedisInsight/pull/4567/files
here how I see it but may be there another way to do this

Kristiyan Ivanov and others added 2 commits May 20, 2025 15:24
…nvironment-variable-to-skip-the-EULA-screen-suggestion

RI-7091 rework repository
} catch (e) {
if (e.code === 'SQLITE_CONSTRAINT') {
return this.getOrCreate();
return this.getOrCreate(sessionMetadata);
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is a retry, shouldn't it pass the options as well?

Suggested change
return this.getOrCreate(sessionMetadata);
return this.getOrCreate(sessionMetadata, defaultOptions);

Copy link
Collaborator

@ArtemHoruzhenko ArtemHoruzhenko left a comment

Choose a reason for hiding this comment

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

lgtm

@KIvanow KIvanow merged commit 38f85ad into main May 20, 2025
46 of 47 checks passed
@KIvanow KIvanow deleted the feature/RI-7091---Add-an-environment-variable-to-skip-the-EULA-screen branch May 20, 2025 15:24
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.

4 participants