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

New behaviour for selects #269

Merged
merged 3 commits into from
Jun 14, 2022
Merged

New behaviour for selects #269

merged 3 commits into from
Jun 14, 2022

Conversation

iamdanchi
Copy link
Contributor

@iamdanchi iamdanchi commented May 20, 2022

image

image

image

image

image

image

Copy link
Collaborator

@theycallmehero theycallmehero left a comment

Choose a reason for hiding this comment

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

Please see comment. We can discuss also, what would be nice API.

@theycallmehero
Copy link
Collaborator

theycallmehero commented May 24, 2022

Basic motivation for this response:

  1. Moon should not know anything about content it renders. It renders content. Having Icon or Logo or whatever knowledge is too much.
  2. Moon Dropdown supports left_icon and right_icon already

Missing totally: content slot for MultiSelect for passing Dropdown with left_icon for example

Working example for Dropdown can be seen at:
https://github.com/coingaming/moon/blob/main/lib/moon_web/pages/components/select/dropdown_page.ex#L82-L92

This does basically everything that is required?

Now, what was missing. Inside of MultiSelect (https://github.com/coingaming/moon/blob/main/lib/moon/components/select/multi_select.ex#L149) there is <:content><:content> block. That needs to be Dropdown or slot with previous links content.

Now it basically works? (Like 3 lines of code for if-else added?)

Needs change: options prop

Additional feature. From your Affiliates code PR I see, that you try to add some props to options, so that dropdown visuals is generated from that.

Links:

  1. https://github.com/coingaming/affiliates_flask/pull/173/files#diff-fb60a958ecdd0bc45cf4249f3cbd436081bac555836fd4ad0b3ac6ee9b719d4aR33
  2. https://github.com/coingaming/affiliates_flask/pull/173/files#diff-5f454592c1693e18e3673fa14eed201169300d552c952a2a74b642f27fcb497bR76
icon: if(model.site.uid, do: prepare_logo_name(model.site.uid), else: "")

I think we could support this, but prefered naming for options prop would be:

 %{
        label: model.site.uid,
        value: model.id,
        left_icon_component: SomeLogoOrWhatEverComponent
  }

And this render to proper place:

<.live_component module={option.left_icon_component} id={option.value} />

Recommendations for future

Start from documentation Page example. In our case, it could have been MultiSelectPage improvement to support left_icon and right_icon example, based on DropdownPage example with left_icon or right_icon.

@iamdanchi iamdanchi force-pushed the hotfix/selects branch 3 times, most recently from b6a8c97 to 6c83e08 Compare June 1, 2022 09:36
"text-trunks-100": !@current,
"w-10": @size == "medium",
"w-12": @size == "large"
"left-0",
Copy link
Collaborator

@theycallmehero theycallmehero Jun 2, 2022

Choose a reason for hiding this comment

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

We wrote 100% working code with flex and gap? Why that code is not used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We wrote code, where bad position for for right_icon, because right_icon should be anchored to the edge of the element. So I reworked it again.

In your implementation, need to add classes for the right_icon object in the passed value, and not set in the template of the called module. I think it's bad, because need to do it on every call

Copy link
Collaborator

@theycallmehero theycallmehero Jun 6, 2022

Choose a reason for hiding this comment

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

I do not know even how to address this. Have we talked, that you should read TailwindCSS and understand it?

It makes me relatively sad, I show how it works, but you decide, that "Nah, I do it other way and I do not ask help, why Margus did show me".

If it happens once, that is ok, but it is kind of common pattern and makes me "wtf" already.

https://tailwindcss.com/docs/flex-grow

Can you restore back to the code that we wrote togeather (for a reason)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, I see. I returned it the way we did

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@iamdanchi
Copy link
Contributor Author

@margusp2heathmont please look on it.
Two question

  1. About icon/logo. If we use it with call render function, we have a problem with default values (described in the prop are don't apply) Also it's not rerendered (I don't know why it happened)
  2. What we do with flags? I prefer the same behavior as with the icon/logo. But where to get them? can be generated from charlists like regular emoticons. unfortunately I did not find how it was done in the moon-design

@iamdanchi iamdanchi changed the title New behaviour for selects [WIP] New behaviour for selects Jun 7, 2022
@iamdanchi iamdanchi force-pushed the hotfix/selects branch 2 times, most recently from 6f11316 to 096b6cf Compare June 13, 2022 14:14
@iamdanchi iamdanchi changed the title [WIP] New behaviour for selects New behaviour for selects Jun 13, 2022
/>
{/if}

<!--
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why there are comments in code?

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 should work with classes, but it doesn't work on my side
Maybe you know how to fix it

<:example>
<div class="flex flex-col gap-4">
<Form for={@user_changeset} change="form_update" submit="form_submit">
<Field name={:role}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why there are 3 same forms?

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's 3 same forms with various fillings

  • label
  • inner label
  • inner label + with icon

Copy link
Collaborator

@theycallmehero theycallmehero left a comment

Choose a reason for hiding this comment

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

Good work!

Please help to understand, added few questions.

Copy link
Collaborator

@theycallmehero theycallmehero left a comment

Choose a reason for hiding this comment

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

Very good work!

Thank you and approved.

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