Skip to content
This repository has been archived by the owner on Apr 25, 2024. It is now read-only.

[DIR-712] - add inputWithButton stories for some forms #428

Conversation

porleaaron
Copy link
Collaborator

What's done

  1. Fix styling of inputWithButton component
  2. Add stories for some existing forms

What needs to be discussed

@linear
Copy link

linear bot commented Aug 2, 2023

DIR-712 Form story and InputWithButton style fix

As visible in the screenshot, the InputWithButton component doesn't line up nicely with the other inputs when used in a form.

Screenshot from 2023-08-01 10-20-31.png

This can easily be fixed by adding className="w-full p-0" to InputWithButton.

It would be good to have a story in storybook where we can see the different input components that we have in context. This will make it easier in the future to spot layout problems and try out improvements.

So please create a form, maybe based on src/pages/namespace/Settings/Registries/Create.tsx or a similar form in our code. It shouldn't be in a modal, but maybe put it in a parent div that has a limited width.

It would be good to have all the different input/form components in it that we have already defined.

You can also look at src/componentsNext/NamespaceCreate/index.tsx in branch sebastianarmbrust/dir-471-mirror, which also uses a Select component in the form (under Auth type) — but this branch is WIP, so don't branch off from it.

Screenshot from 2023-08-01 10-08-01.png

UPDATE:

In the branch sebastianarmbrust/dir-471-mirror, I am using it with a Tooltip (will commit later). This use case should also be documented in the new story.

@porleaaron porleaaron requested a review from sebxian August 2, 2023 04:24
Copy link
Collaborator

@sebxian sebxian left a comment

Choose a reason for hiding this comment

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

Thanks, @porleaaron !

I noticed a few things that were not clear to me from the start, so I couldn't give more detailed instructions. It would be great if you could look at the following two topics and make the respective changes:

Storybook folder structure

  • Good idea to add the InfoTooltip as a general component! That means we should also have a story for that component (so it can be found in the storybook list). Could you please add this?
  • I just talked to Stefan and we decided we should have one storybook folder where we collect examples on how to combine components. So could you please do the following:
    • make a top-level folder called "Composition" in storybook
    • add a sub-level folder "Forms" and move the form examples here (they should not be under the InputWithButton).

Button layout

I noticed the "submit" and "cancel" buttons are full width in the one form example you added (probably they are layout: box in CSS?).

In the real app, we have them side by side. In these cases, the form is wrapped in a <Dialog> and the <DialogFooter> controls the button layout.

I am not sure how to solve this, but I think it would be good to have the same layout in storybook as well. So can you please find out:

  • is it possible to use the <Dialog> or <DialogContent> component as a wrapper in storybook without a "real" popover dialog? In this case, the example would better reflect the real world use case.
  • As an alternative, it is probably easier just to add a wrapper around the buttons that groups them next to each other (similar to the examples in the Buttons stories). This would also be a helpful example to have in storybook.

It would be great if you could experiment with these options and report back which one works best and makes the most sense.

Let me know if there are any questions!

<TooltipProvider>
<Tooltip delayDuration={100}>
<TooltipTrigger type="button">
<Info size={16} className="ml-2 text-gray-11" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<Info size={16} className="ml-2 text-gray-11" />
<Info size={16} className="mx-2 text-gray-11" />

It does not make a difference visually, but I think it makes more sense to change this to "mx-2". (This was my own mistake in the code you copied.)

@porleaaron
Copy link
Collaborator Author

Yeah, thanks for your feedback
I will work for those comments

Copy link
Collaborator

@sebxian sebxian left a comment

Choose a reason for hiding this comment

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

Thanks for the updates, @porleaaron 👍

I decided to postpone reviewing and merging this in detail, because I am not sure exactly how we should organize these examples in storybook and we do not have the time to make good decisions before we have met our milestones for the current release.

So I will leave this open for now and update the ticket in linear accordingly.

@stefan-kracht stefan-kracht changed the base branch from feature/redesign to develop October 20, 2023 08:33
@sebxian sebxian changed the base branch from develop to main January 12, 2024 14:45
@sebxian
Copy link
Collaborator

sebxian commented Jan 15, 2024

Closing this. "Composition" stories for forms make sense, but more work is required to build forms that are a useful template for development (with working examples of all components we have developed so far).

@sebxian sebxian closed this Jan 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants