-
Notifications
You must be signed in to change notification settings - Fork 21
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
feat(cc-logs-control): init component #909
Conversation
🔎 A preview has been automatically published : https://clever-components-preview.cellar-c2.services.clever-cloud.com/cc-logs-controller/init/index.html. This preview will be deleted once this PR is closed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done @pdesoyres-cc this component looks great and the code is easy to read 👍
Most of my comments are nitpicks as usual and here is one more:
- I think we should change the icon for the follow button. I always think I'm going to be able to download the logs and since this button is icon only, we should avoid ambiguous icons. I couldn't find a really good fit within the remix icons project but the scroll down examples form the Noun Project look decent enough to me. Maybe we could create an issue within the Remix project?
8687efc
to
42ac00f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GG @pdesoyres-cc ! 👏
The visual is very nice ! ✨ I went through the code and I have just left one question. Also about the cc-logs-controller
, when I saw the scroll to bottom icon I though it was a button to download the logs. I haven't noticed it before and I don't know if I am the only one to have this impression or if there is another icon more appropriate. Otherwise, LGMT !
src/components/cc-logs-controller/cc-logs-controller.stories.js
Outdated
Show resolved
Hide resolved
5bfb534
to
60178ab
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @pdesoyres-cc, good job on this!! 👏
I added a few remarks and here are some global notes:
- Now that I see the component, I'm not sure
cc-logs-options
is the right name - Will we put the future input for text filtering in this component or through the slot?
- I was about to say "why do we need this component, why is it not directly in the main component ?"
- Looking at the code, I understand why and it's a good way to isolate stuffs, even if it means mirroring a lot of properties
- Can we have a story to show of the fact that we can select other options than the default ones?
src/components/cc-logs-controller/cc-logs-controller.stories.js
Outdated
Show resolved
Hide resolved
src/components/cc-logs-controller/cc-logs-controller.stories.js
Outdated
Show resolved
Hide resolved
src/components/cc-logs-controller/cc-logs-controller.stories.js
Outdated
Show resolved
Hide resolved
60178ab
to
854865a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your feedbacks and new commits 👍
I still have questions:
- I'm not sure
cc-logs-options
is the right name but I don't wantcontroller
either, WDYT? - Will we put the future input for text filtering in this component or through the slot?
- Can we have a story to show of the fact that we can select other options than the default ones?
My first idea for the slot was to put the date range input, but there was not enough room. Having said that, we could get rid of the slot and plan the addition of the text-filtering input.
yes, I'll do that. |
854865a
to
34af45c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work Pierre, LGTM! 💪
After a discussion with @pdesoyres-cc
|
34af45c
to
b4d51fa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Congrats for the awesome work: there's an issue with a non-working story but LGTM for the rest!
0c6475a
to
cd63fbb
Compare
cd63fbb
to
24e6768
Compare
🔎 The preview has been automatically deleted. |
Fixes #908
What does this PR do?
3 commits:
How to review?
A review from everyone would be great.