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

Change helpText from prop to slot in NameAndDescInputModalForm #1858

Closed
rajatvijay opened this issue Oct 21, 2022 · 7 comments · Fixed by #1903
Closed

Change helpText from prop to slot in NameAndDescInputModalForm #1858

rajatvijay opened this issue Oct 21, 2022 · 7 comments · Fixed by #1903
Labels
good first issue Everything in "Help wanted", PLUS being relatively easy and straightforward to implement. help wanted Community contributors can implement this ready Ready for implementation type: enhancement New feature or request work: frontend Related to frontend code in the mathesar_ui directory
Milestone

Comments

@rajatvijay
Copy link
Contributor

rajatvijay commented Oct 21, 2022

helpText would work better as a slot than as a prop. I see in the parent component you're passing a huge string into this prop. If you use a slot, you'll be able to move that text out of a string and put it directly into the template. This also makes it possible to put formatting in the help text.

File: mathesar_ui/src/components/NameAndDescInputModalForm.svelte

Originally posted by @seancolsen in #1793 (comment)

@rajatvijay rajatvijay added good first issue Everything in "Help wanted", PLUS being relatively easy and straightforward to implement. help wanted Community contributors can implement this work: frontend Related to frontend code in the mathesar_ui directory ready Ready for implementation labels Oct 21, 2022
@rajatvijay rajatvijay added this to the 2023 or later milestone Oct 21, 2022
@rajatvijay rajatvijay mentioned this issue Oct 21, 2022
7 tasks
@kgodey kgodey added the type: enhancement New feature or request label Oct 21, 2022
@KAIMonmoy
Copy link
Contributor

I would like to work on it, if it is still open. @rajatvijay could you please assign this to me?

@seancolsen
Copy link
Contributor

@KAIMonmoy Thanks! Feel free to start work on this ticket and submit a PR when you're ready. We are non longer marking external contributors as officially "assigned" to tickets though because it's been challenging managing the people who don't follow through.

@KAIMonmoy
Copy link
Contributor

Thank you for your response, @seancolsen. I'll start working on this ticket.
It might take some time as I'm quite new to open-source contribution 😅. But I'll definitely try my best.

@KAIMonmoy
Copy link
Contributor

KAIMonmoy commented Nov 4, 2022

Hey @seancolsen ,
I don't think just converting helpText from prop to slot would do the work.

Because when we're passing helpText as a prop we can conditionally render the Alert Component in File: mathesar_ui/src/components/NameAndDescInputModalForm.svelte

But when we are passing helpText as a slot, we'll have to update the code in File: mathesar_ui/src/components/NameAndDescInputModalForm.svelte like below -

- {#if helpText}
-   <Alert appearance="info">
-     <slot slot="content">{helpText}</slot>
-   </Alert>
- {/if}
+ {#if $$slots.helpText}
+   <Alert appearance="info">
+     <slot name="helpText" slot="content" />
+   </Alert>
+ {/if}

But now in order to make it work properply we'll have to change the code in File: mathesar_ui/src/pages/database/AddEditSchemaModal.svelte like below -

  saveButtonLabel={schema ? 'Save' : 'Create New Schema'}
-  helpText={schema ? '' : createSchemaHelpText}
 >

+  {#if !schema}
+    <svelte:fragment slot="helpText">{createSchemaHelpText}</svelte:fragment>
+  {/if}

But the problem is we cannot have a slotted component inside an if.

I can think of two solutions to overcome this issue.

(1) Taking the Alert component to File - mathesar_ui/src/pages/database/AddEditSchemaModal.svelte and using a slot to render it in File - mathesar_ui/src/components/NameAndDescInputModalForm.svelte

// mathesar_ui/src/components/NameAndDescInputModalForm.svelte

- {#if helpText}
-   <Alert appearance="info">
-     <slot slot="content">{helpText}</slot>
-   </Alert>
- {/if}
+ {#if $$slots.helpText}
+   <Alert appearance="info">
+     <slot name="helpText" slot="content" />
+   </Alert>
+ {/if}
// mathesar_ui/src/pages/database/AddEditSchemaModal.svelte

- import type { ModalController } from '@mathesar-component-library';
+ import { Alert, ModalController } from '@mathesar-component-library';
  
...

 saveButtonLabel={schema ? 'Save' : 'Create New Schema'}
-  helpText={schema ? '' : createSchemaHelpText}
 >

...

   {#if schema}
      Rename <Identifier>{initialName}</Identifier> Schema
   {:else}
     Create Schema
+     <Alert appearance="info" slot="helpText">
+        <span slot="content">{createSchemaHelpText}</span>
+     </Alert>
   {/if}

(2) We can extract out Alert along with the helpText slot and make it into a component called NameAndDescInputModalFormHelp text. And use that in place of the Alert component in File - mathesar_ui/src/pages/database/AddEditSchemaModal.svelte

Let me know which one you prefer. And also if you think I am missing something or there are other ways to solve this problem, please share those with me.

@KAIMonmoy
Copy link
Contributor

One more thing, which one do you think looks better?

  1. image
  2. image

@seancolsen
Copy link
Contributor

Ugh. You've run into sveltejs/svelte#5604. I've hit this before and it's annoying.

Here's the way I'd like to fix this...

  • In NameAndDescInputModalForm.svelte we should have only

    <slot name="helpText" />

    (We should should not be rendering Alert.)

  • Also, in Alert.svelte, we should should change <slot name="content" /> to <slot /> because there is only one slot. We'll need to update the other reference to this component too.

  • Then in AddEditSchemaModal.svelte, we should have:

    <svelte:fragment slot="helpText">
      {#if !schema}
        <Alert appearance="info">
          Name your schema to reflect its purpose. For example, your personal
          financial schema may be called "Personal Finances" and your movie
          collection "Movies." Add a description to your schema to remember what
          it's for.
        </Alert>
      {/if}
    </svelte:fragment>

That should do the trick. You shouldn't need to mess with the position of the light bulb in Alert.svelte.

@KAIMonmoy
Copy link
Contributor

@seancolsen I have opened a PR-1903. That should close fix/close this issue. Thanks for all the help.

Repository owner moved this from Ready to Done in [NO LONGER USED] Mathesar work tracker Nov 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Everything in "Help wanted", PLUS being relatively easy and straightforward to implement. help wanted Community contributors can implement this ready Ready for implementation type: enhancement New feature or request work: frontend Related to frontend code in the mathesar_ui directory
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

4 participants