railsdevs.com
railsdevs.com copied to clipboard
joemasilotti
Lookbook
Lookbook integration. Resolves #624
Pull request checklist
- [ ] My code contains tests covering the code I modified
- [ ] I linted and tested the project with
bin/check - [ ] I added significant changes and product updates to the changelog
@joemasilotti little update here, I couldn't advance much last week, I'll continue with this today.
Sounds great ā thanks for the update! Excited to see what's next. :)
What is pending here? Need some help @faqndo97 ? I've seen your branch and is pretty cool. I want to get this feature merged, because it's really cool to work with components.
@KarlHeitmann thanks for asking, Iām planning to finish last details in the upcoming days and open the PR to review it.
@joemasilotti this is ready for the first formal code review when you can šš¾
Thanks for your patience on this ā I just got back from The Rails SaaS Conference and finally have time to dive in.
I pulled down the code and did a quick visual review of the components. They are looking awesome!
A few questions/suggestions:
- The small components look odd when viewed alone. I think we need our default layout to add a bit of padding to give them some room to breath.
- Let's use sentence case for all the parameters ("Title" instead of "title" and "Some title" instead of "Some Title").
- The larger components (forms + tables) could also use some padding and probably a max width. Right now they look a little odd stretched out to fill the entire screen.
- I'm not sure how I feel about having versions of the same component that don't look visually (e.g. the
idfield forList). I'm thinking we might want to only show the differences for visual components and let the developer discover the remainder of the options themselves. - Let's use the name of the component in the component's default copy. For example, in the form container we can use "Form container" for the title and describe when/how to use it in the description. I think that will show a better idea of how it looks with more realistic copy.
I haven't dug into the code just yet but wanted to give you some feedback first.
Outside of this, are there any more components that need to be added or are these all of them?
Hey again @faqndo97. Do you still have appetite to get this over the finish line? I'm currently working through a redesign for the marketing pages of RailsDevs. And I'd love to use Lookbook to show off the new colors and components.
Let me know! And no pressure if not āĀ I'm happy to take over. There is already a solid base and I don't think I would have much more to do.
Hey @faqndo97! I haven't heard from you in over a month so I'm going to take over this PR. If anything changes let me know.
@joemasilotti Sorry for the big delay on this, my life have been a mess and with bunch of changes since my last commit. Can I help you to wrap this up?
No need to apologize. I clearly didn't make any progress either. š
Yes, please! I would love some help getting this over the finish line.
Hey @faqndo97, do you still have appetite to get this over the finish line?
No worries if not! But let me know this week so I can assign it to someone else or finish it myself.
Hey guys!
@faqndo97 need some help there? I've fixed the issues @joemasilotti mentioned tonight I may have some time to research and answer the last question here
A heads up that Lookbook v2 was just announced. So we will probably also want to upgrade to that for this PR. š«¤
@faqndo97 are you still interested in this PR? If not, let's chat if we can pass it of to @KarlHeitmann.
@joemasilotti Thanks for asking! I can tackle the review comments and update lookbook version this weekend if you're interested. Let me know
@joemasilotti Remaining items:
I'm not sure how I feel about having versions of the same component that don't look visually (e.g. the id field for List). I'm thinking we might want to only show the differences for visual components and let the developer discover the remainder of the options themselves.
Which ones do you suggest to remove? I feel them useful except Basic Link for example that are literally the same with more params. They can be merged into the one with more params. The same can apply for List with item. Apart from that IDK which others are redundant. Do you have other suggestions?
Let's use the name of the component in the component's default copy. For example, in the form container we can use "Form container" for the title and describe when/how to use it in the description. I think that will show a better idea of how it looks with more realistic copy.
About this not all components have a clear place to put the component name, for example the table component. We can use this one maybe https://lookbook.build/guide/previews/grouping/#grouping-all-examples or use the notes section with a title being the component name and the description digging deeper on the component usage. What do you think?
A heads up that Lookbook v2 was just announced. So we will probably also want to upgrade to that for this PR. š«¤
The current stable version is 1.5.3 and V2 is in beta, do you still want to use the beta version here?
Ignore my comment for elements that don't have a place for text to appear.
And judging by the minimal code change requirements outlined in the docs I vote we push for v2.
@joemasilotti Ok, I'll update to the new version until Tuesday so we can wrap this PR this week!
Hey @faqndo97 and @KarlHeitmann! Do either/both of you have appetite to get this over the finish line?
RailsDevs is getting a makeover soonish and I'd love to have Lookbook in place before then if possible.
Hey @joemasilotti Unfortunately, until May I will be quite busy as I am changing jobs. However, the theme of the components and the lookbook gem interest me greatly. I look forward to contributing to this as soon as I have more free time.
Hey @faqndo97, I'm doing a bit of spring cleaning on the repo and closing out old PRs. I still would love to see this integrated but I want to wait for the redesign, first.