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

feat: switch to react-component #166

Merged
merged 8 commits into from
May 6, 2021

Conversation

magicmatatjahu
Copy link
Member

@magicmatatjahu magicmatatjahu commented Mar 18, 2021

Description

Switch to React component:

  • remove code related to Nunjucks (remove something like 90% of template 😅).
  • unfortunately template is still written in Nunjucks, not in React, due to React templates make generation process longer generator#521 issue. After fixing it, rewriting ro React will be very easy.
  • write scripts/copy-sources.js script to copy to template's files all needed sources to use template:
    • react-dom
    • react
    • asyncapi-react component
    • default (also minified) styles for react-component
  • update Readme.md
  • write needed filters and tests for them
  • update hooks

The easiest way to test template from PR is run generator by:

ag https://gist.githubusercontent.com/derberg/4e419d6ff5870c7c3f5f443e8bd30535/raw/6188903caf77ad14ee32e22b0c86bf6fa244b580/asyncapi-websocket-kraken.yml https://raw.githubusercontent.com/asyncapi/html-template/989c2ba26f228e030550dea285bb09bcdad92b57/asyncapi-html-template-0.20.1.tgz -o ./sample --force-write

Don't afraid 😅 I didn't put any virus to .tgz. You can check that this package is inside the PR. Of course I will remove it before merging to master.

Related issue(s)
Part of asyncapi/shape-up-process#88

Blocked by asyncapi/shape-up-process#86
Blocked by asyncapi/asyncapi-react#301
Blocked by asyncapi/asyncapi-react#302

Fixes #168
Fixes #167
Fixes #15
Closes #160
Closes #51

@magicmatatjahu magicmatatjahu added the enhancement New feature or request label Mar 18, 2021
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide and the instructions about a basic recommended setup useful for opening a pull request.

Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.

Copy link
Member

@fmvilas fmvilas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exciting to see this coming! 😄

filters/all.js Show resolved Hide resolved
template/index.html Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@magicmatatjahu
Copy link
Member Author

magicmatatjahu commented Apr 20, 2021

@derberg Thanks!

I added tests for new filters.

contribution flow will not be super friendly now, for now please at least make sure react component has https://github.com/asyncapi/generator/blob/master/.github/workflows/bump.yml flow to update dependants automatically

Good catch! I will add it, because component hasn't it.

we need a PR for playground too

I have PR asyncapi/asyncapi-react#301 in React component with styling vol.2 (for examples and sidebar) and probably only one possibility is fetch my fork with my branch and run playground locally. A nice solution will be connect Netlify to our playground, I will create issue for that.

@magicmatatjahu
Copy link
Member Author

@derberg Issue is created asyncapi/asyncapi-react#306

@fmvilas
Copy link
Member

fmvilas commented Apr 21, 2021

Code-wise, it looks great to me. I'm only seeing a problem with the expand/collapse buttons not being clickable (only the chevron icon is). Here's a video of what I mean: https://www.loom.com/share/3b7037ddd3074796a4774dbfe52790e4

@magicmatatjahu
Copy link
Member Author

@fmvilas Thanks for review! :) I left one PR in react-component repo for such bugs. I will correct it! :)

@magicmatatjahu
Copy link
Member Author

magicmatatjahu commented Apr 22, 2021

@fmvilas Fixed! You can check it by command:

ag https://raw.githubusercontent.com/asyncapi/generator/master/test/docs/dummy.yml https://raw.githubusercontent.com/asyncapi/html-template/6714302c5276bd3b7c575e23bb1445d83312bdc3/asyncapi-html-template-0.20.1.tgz -o ./sample --force-write -p version='1.2.5' -p sidebarOrganization=byTagsNoRoot -p singleFile=true

Here is a new tgz:

https://raw.githubusercontent.com/asyncapi/html-template/6714302c5276bd3b7c575e23bb1445d83312bdc3/asyncapi-html-template-0.20.1.tgz

Changes are from: asyncapi/asyncapi-react#310

@magicmatatjahu magicmatatjahu requested a review from derberg April 22, 2021 10:18
Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have PR asyncapi/asyncapi-react#301 in React component with styling vol.2 (for examples and sidebar) and probably only one possibility is fetch my fork with my branch and run playground locally. A nice solution will be connect Netlify to our playground, I will create issue for that.

There seems to be a misunderstanding here 😄 I mean that once this is released, you need to manually update it here https://playground.asyncapi.io/ and make sure it works


I did manual testing with:

also this anonymous-message-1 should not be there 🤔 maybe my messages do not have name prop or title but they are in components and the key name should be applied right?
Screenshot 2021-04-22 at 13 32 58


overall though, super great job mate, I love that when there are objects in extensions and bindings, you do not just dump but try rendering in a nice way ❤️

@magicmatatjahu
Copy link
Member Author

@derberg Thanks for review man!

overall though, super great job mate, I love that when there are objects in extensions and bindings, you do not just dump but try rendering in a nice way ❤️

🍻

I will address all bugs what you found, thanks you again for that! Some are very strange like description is super narrow... Probably I missed about one css class when I was splitting this PR.

https://bitbucket.org/qbmt/zbos-mqtt-api/raw/25ee18cee0db90d9f370e0bcf1bfbb6fbb7621ce/zbos_mqtt-all-asyncapi.json and when you have loads of messages (over 160) then expanding collapsable elements have delay, but yeah, not a blocker but something to think about in future

Hmmm... I tried write collapse button in this way that shouldn't be rendered in each collapse... Examples haven't this delay 🤔
But yeah, I will create issue for that.

also this anonymous-message-1 should not be there 🤔 maybe my messages do not have name prop or title but they are in components and the key name should be applied right?

Strange, I will check that!

@fmvilas
Copy link
Member

fmvilas commented Apr 22, 2021

Lovely! Yeah, I agree with @derberg. The only part that can be more challenging is taking the key on the components section as the name of the message because by the time you're processing the document it's already dereferenced and you don't have the context. Anyway, that's a problem to solve in the parser too, not a blocker for me.

@derberg
Copy link
Member

derberg commented Apr 22, 2021

I noticed it only because in the current playground I don't see those anonymous tags

so for me a blocker as it is breaking existing functionality and this should not happen

@magicmatatjahu
Copy link
Member Author

magicmatatjahu commented Apr 22, 2021

I checked this Kraken API in my local React playground with newest component and it renders like:

image

Probably there is a problem with hydration.

derberg
derberg previously approved these changes Apr 26, 2021
@magicmatatjahu magicmatatjahu dismissed stale reviews from derberg and fmvilas via 37d1346 April 26, 2021 16:16
@magicmatatjahu
Copy link
Member Author

magicmatatjahu commented May 4, 2021

@derberg @fmvilas I updated dependency of react-component to 1.0.0-next.1 :) Here is a tgz to test:

https://raw.githubusercontent.com/asyncapi/html-template/2aff6fba4732c88d414aa05964599a70b28793a4/asyncapi-html-template-0.20.1.tgz
ag https://gist.githubusercontent.com/derberg/4e419d6ff5870c7c3f5f443e8bd30535/raw/6188903caf77ad14ee32e22b0c86bf6fa244b580/asyncapi-websocket-kraken.yml https://raw.githubusercontent.com/asyncapi/html-template/2aff6fba4732c88d414aa05964599a70b28793a4/asyncapi-html-template-0.20.1.tgz -o ./sample --force-write -p version='1.2.5' -p sidebarOrganization=byTagsNoRoot -p singleFile=true

So finally, we can merge PR 🚀

Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this one be next release too? in the end it now relies fully on react component, structure of generated files changes, and when react reaches 1.0, then this template will reach 1.0 too, right?

open question here

@derberg
Copy link
Member

derberg commented May 4, 2021

also separate topic, but I guess we should consider moving this repo as part of react component monorepo?

@magicmatatjahu
Copy link
Member Author

@derberg With next releases, it is a good question. I'm also open here. @fmvilas What do you think?

also separate topic, but I guess we should consider moving this repo as part of react component monorepo?

I had a lot of problems with testing react-component in html-template to have 100% sure that everything works, I mean making symlinks, sometimes symlinks were broken, so I had to reinstall deps etc... so I like your idea :) But we must ask the core contributors to this repo, what about it they think.

@derberg
Copy link
Member

derberg commented May 4, 2021

I had a lot of problems with testing react-component in html-template to have 100% sure that everything works, I mean making symlinks, sometimes symlinks were broken, so I had to reinstall deps etc... so I like your idea :) But we must ask the core contributors to this repo, what about it they think.

yeah, I think any maintainer would prefer to have it in one repo instead, but for now forget about it, I forgot generator would not be able to use such template, it doesn't support monorepo

@magicmatatjahu
Copy link
Member Author

yeah, I think any maintainer would prefer to have it in one repo instead, but for now forget about it, I forgot generator would not be able to use such template, it doesn't support monorepo

We can make similar approach like in web-component so user will be able to use template like now by @asyncapi/html-template. What is a problem here? Maybe I don't know about something 😅

@derberg
Copy link
Member

derberg commented May 4, 2021

@magicmatatjahu I quite often install template from someones fork, or mine fork using something like https://github.com/USERNAME_WHERE_FORK_IS/html-template#BRANCH_NAME

@magicmatatjahu
Copy link
Member Author

magicmatatjahu commented May 6, 2021

@fmvilas What idea do you have? Together with Łukasz, we are open to suggestions. Go with 1.0.0-next or publish html as 0.21.0?

@jonaslagoni @smoya Also you guys, what do you think?

@fmvilas
Copy link
Member

fmvilas commented May 6, 2021

I have no preference, to be honest. Either way would be fine for me.

@jonaslagoni
Copy link
Member

Same as Fran, no preference here, especially since the template is < v1.

@sonarqubecloud
Copy link

sonarqubecloud bot commented May 6, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@magicmatatjahu magicmatatjahu requested a review from derberg May 6, 2021 13:29
@magicmatatjahu magicmatatjahu merged commit e7d8952 into asyncapi:master May 6, 2021
@magicmatatjahu magicmatatjahu deleted the react-integration branch May 6, 2021 13:44
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 0.21.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@magicmatatjahu
Copy link
Member Author

@all-contributors please add @magicmatatjahu for code, test, bug

@allcontributors
Copy link
Contributor

@magicmatatjahu

I've put up a pull request to add @magicmatatjahu! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released
Projects
None yet
5 participants