[#17727] Implement Dropdown input component #17927
Conversation
Jenkins BuildsClick to see older builds (34)
|
|
just wondering how's it different from |
| :margin-bottom gap}) | ||
|
|
||
| (def root-container | ||
| {:width dropdown-width}) |
|
@flexsurfer i know what you mean — the diff between this component and the original dropdown is thin, it mostly about different use cases. Nevertheless team decided that we should have a separate dropdown-input component for this particular case and that it would be better not to re-use the original dropdown. is that correct? @mariocnovoa |
tumanov-alex
force-pushed
the
17727-dropdown
branch
2 times, most recently
from
55819f3 to
e23ba5b
Compare
|
Yep, we follow the hierarchy that is set in figma. Otherwise we try to get changes shoehorned into components and especially if the designs change. If there is some issues with this approach it's best to discuss with designers to update their usage of the components. The less components we have to support the better 👌 |
|
@J-Son89 initially i've had concerns about copy-pasting dropdown code (its not dry i thought) but when you consider how difficult it would be to support a component that's basically 1.5 components, you realize it's better to have two simple modules that one, but complex and bug prone. |
There was a problem hiding this comment.
@tumanov-alex, @J-Son89, I'm not too concerned about the component duplication because the root of the problem is in the design system (on the Figma layer).
Our design system is full of "almost identical" things, full of one usage components. The dropdown is not the only one (e.g. selectors and tags components). It would be way better for maintainability, especially at MVP stage if designers (and devs in turn) judiciously applied something like the atomic design methodology.
I'm not saying we can't do anything to keep things DRY, of course we can, but I think in the end it's an uphill battle. And changing that landscape would require a complete design shift, both in mindset and in the actual components that would be built as legos.
But having said that, it's unclear to me if we shouldn't at least reuse some parts between both components. I'm approving anyway because I don't think it's worth refactoring to be more DRY if you already have something working.
@ilmotta, I agree. The issue is at the source (figma). Indeed it is too late to change what is already in place. We should however bring this up with designers as they go to add new components/designs. Let's raise this as a point of discussion with them as I believe browser is currently being designed. |
| [quo.foundations.colors :as colors])) | ||
|
|
||
| (def gap 8) | ||
| (def dropdown-width 288) |
There was a problem hiding this comment.
I think we don't want to set width. It's better to use flex and then we set margin on the screen where the component is being used.
Usually via a 'container-style' prop which is passed to the outer container
There was a problem hiding this comment.
if you mean setting flex 1 to root-container & container, then we'll have an issue with height also being increased as component will grow. I've used width 100% combined with height 40px, if that's ok
| label? true | ||
| blur? false} | ||
| :as props} | ||
| label header-label] |
There was a problem hiding this comment.
Any particular reason label and header label aren't passed in the map?
There was a problem hiding this comment.
to be honest i thought we have a convention to pass such info as labels outside of maps to imitate api of core components like rn/text, where you also pass label outside of map
There was a problem hiding this comment.
yeah that's true, I think I had not seen any component with 2 args like this, i.e [map arg arg] but most components like button, dropdown etc take the form of [map arg].
I don't. think it matters so much, I wonder for consistency is it better to put header-label in the map instead? how does the other dropdown handle it, let's match that if it has a similar header-label?
| @@ -232,6 +233,7 @@ | |||
| ;;;; Dropdowns | |||
| (def dropdown quo.components.dropdowns.dropdown.view/view) | |||
| (def network-dropdown quo.components.dropdowns.network-dropdown.view/view) | |||
| (def dropdown-input quo.components.dropdowns.dropdown-input.view/view) | |||
There was a problem hiding this comment.
Can we keep this alpha sorted, makes this file easier to keep organised/ check what's there 👍
There was a problem hiding this comment.
for some reason i get no error from lint about this 🤔
Let's do it 👍🏼 The important part for me is that we (but especially designers) understand the first principles behind this problem and how they propagate to code as inefficiencies and bugs. We just need to make sure the discussion doesn't focus on only one or two particular examples, which is what usually happens. |
|
Probably it would help if some developers could review the components before they're "finalised" too |
tumanov-alex
force-pushed
the
17727-dropdown
branch
4 times, most recently
from
ca8402f to
84fe10b
Compare
| header-label "Label"} | ||
| :as props} | ||
| label] | ||
| (let [icon-size 20 |
There was a problem hiding this comment.
We can move this value to the properties ns/file and use it from there.
tumanov-alex
force-pushed
the
17727-dropdown
branch
from
84fe10b to
a1f7454
Compare
There was a problem hiding this comment.
Everything looks good besides the label color, please check it 🙏
here's the review
tumanov-alex
force-pushed
the
17727-dropdown
branch
from
4a0144e to
3edbb37
Compare
tumanov-alex
force-pushed
the
17727-dropdown
branch
from
3edbb37 to
e2e94e2
Compare
tumanov-alex
force-pushed
the
17727-dropdown
branch
from
e2e94e2 to
cfb224f
Compare
fixes #17727
Summary
Implement dropdown-input component based on the original dropdown one
Figma
| --- | --- | --- |