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

Ui and code improvements #32

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

Poyoman39
Copy link

Hi @gadicc

Would you review this PR ? It intends to improve the following points:

  • Migrate to function components for a more modern code
  • Add style display contents on dom container so it doesn't impact the visual
  • Add template special prop so it's possible to send a "template" data to blaze templates

@gadicc
Copy link
Owner

gadicc commented Jan 31, 2025

Hey @Poyoman39

Thanks so much for this, all looks great! Actually so weird seeing the original non-hooks code from a decade a back 😅 So very welcome improvements indeed!

I'll be very happy to merge this, just a few small things:

  1. Can I assume this is well tested in your own projects?
  2. Do the tests in the repo work? (TBH, looks like these are still all for Meteor v1, so no worries if not).
  3. Could I just ask you to also amend the README, re the rendered HTML. With the change from Fragment to div, is there still a span somewhere? I don't actually have Meteor setup anywhere to check myself 😅

Thanks so much again for this great improvement in code quality. Very happy that this package is still useful and continued to be improved by the community after all this time! 🙏

@Poyoman39
Copy link
Author

Poyoman39 commented Jan 31, 2025

Hey @gadicc

Thank you for your answer and happy to see my PR his welcome :)

  1. I can't assume it's well tested but "it run on my computer" :D
  2. Well ... shame on me i did not run them, since my machine is configured with meteor 3
  3. I switched back to a span, there was no real reason for this change. Also i removed the classname possibility since it was a problem if children needed a classname ...

What do you think between these solutions would be the best ?

  • Add a constant classname on generated span ? Like "meteor-blaze-react-component-wrapper"
  • Instead of <Blaze {...props} template="atForm" /> do something like <Blaze childrenProps={...props} template="atForm" /> would be acceptable ? This would also solve the template special trick :D (Would really be the cleanest solution making possible to useMemo the childrenProps)
  • Any other idea ? ;)

@gadicc
Copy link
Owner

gadicc commented Feb 3, 2025

Thanks for your reply, @Poyoman39, and sorry for my late reply. I'm just in the middle of some travel at the moment, so will have to be in touch in a few more days when I'm back home. Thanks for your patience :)

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