feat(cc-search-bar): init component#1733
Conversation
08e1189 to
8d3aa7b
Compare
|
🔎 A preview has been automatically published : https://clever-components-preview.cellar-c2.services.clever-cloud.com/feat/search-bar/index.html. This preview will be deleted once this PR is closed. |
pdesoyres-cc
left a comment
There was a problem hiding this comment.
Nice work @clara-layus-cc
cc15826 to
3331f00
Compare
Galimede
left a comment
There was a problem hiding this comment.
Hey Clara, overall mostly nitpicks and thoughts but LGTM, GG! 👏
There was a problem hiding this comment.
Good job @clara-layus-cc the component looks nice!
I have a few questions:
- will there be async loading involved? The console will need the list of all the resources, it already has it in it's summary so I assumed you're going to rely on this? (this would mean no loading I think so it would be fine)
- why not using the
cc-dialogcomponent here? Do you plan to use it from the console directly? - I think we could use the
cc-dialogand let it handle the main heading + close button. Since the wireframes seem to have a smaller padding than the default dialog, we could use the padding custom props to customize these and match the wireframes while still using thecc-dialogcomponent. We can discuss all of that in sync if needed 😉 - the margin / gap between the section label and the border above it does not match the wireframes (margin after each section is fine, but margin before each section is too narrow)
|
Well done, Clara!
I don't think I've left anything out. 💪 Good work, and please don't hesitate to ask me or drop me a line if you have any further questions. |
3331f00 to
7443c04
Compare
roberttran-cc
left a comment
There was a problem hiding this comment.
LGTM, nice component! No additional comments beyond those already raised.
aa612ce to
ec92588
Compare
|
I’ve just had another look, particularly at the text in the two versions —‘empty’ and ‘no result’— and I agree with your suggestions.
Great work again 💪 |
ec92588 to
23dc518
Compare
florian-sanders-cc
left a comment
There was a problem hiding this comment.
A few a11y issues but almost there 👍
23dc518 to
734cc02
Compare
florian-sanders-cc
left a comment
There was a problem hiding this comment.
All good for me, great job @clara-layus-cc 😎
roberttran-cc
left a comment
There was a problem hiding this comment.
Only a minor doc-related feedback for me, LGTM otherwise, hence the approval.
734cc02 to
6b06ed0
Compare
hsablonniere
left a comment
There was a problem hiding this comment.
Well done @clara-layus-cc, I added a small comment, feel free to apply it or not and here are some changes I want :
- The main bloc should be aligned to the top to prevent the input to move while typing.
- The hover with the ">" shifts the content, I think we need to prevent this.
Notes :
- I was wondering if I needed a
clear()method but assigningvalue=""is enough.
322d19d to
ee847bb
Compare
florian-sanders-cc
left a comment
There was a problem hiding this comment.
A small change to make but apart from that it's good to me!
pdesoyres-cc
left a comment
There was a problem hiding this comment.
LGTM. Well done Clara
5d8ba18 to
709a1ee
Compare
Move the URL-origin check out of cc-link as a private method into a shared utility, so other components (cc-search-bar) can reuse it.
709a1ee to
025bfda
Compare
|
🔎 The preview has been automatically deleted. |
What does this PR do?
How to review?