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

Generate API Documents #748

Merged
merged 28 commits into from
Mar 28, 2018
Merged

Conversation

matuzalemsteles
Copy link
Member

Hey @jbalsas, For this PR to be able to move on, it's depending on the pr liferay/electric#139 on the Electric for it to work again.

New

  • Clay API Documentation
    • Transition between different versions of documentation
  • Use electric-clay-components

Disclaimer

The problem of compiling with the Charts components on clayui.com seems to be a particular problem, I've tested it with other components and it seems to work fine, we have to dig deeper to know what it's causing.

The API page is separate from the list of components, this is a bit poor in navigation between the component pages and the API, we can think of adding a topbar to improve navigation and user experience.

The design of the page is very simple, I tried to follow what we see in the alloy-editor feel free to modify later, I think we can improve still.

@coveralls
Copy link

coveralls commented Mar 16, 2018

Pull Request Test Coverage Report for Build 555

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.002%) to 81.907%

Totals Coverage Status
Change from base Build 539: 0.002%
Covered Lines: 4600
Relevant Lines: 4865

💛 - Coveralls

@jbalsas
Copy link
Contributor

jbalsas commented Mar 16, 2018

Hey @matuzalemsteles, I'm getting the soyAPIEntitiesPath error when trying to run this. Could you get a new release of electric out so I can try it out?

How can I test it in the meantime?

@matuzalemsteles
Copy link
Member Author

Hey @jbalsas, I'm expecting the release of Electric, but to work temporarily I'm modifying in node_modules so I can continue working on it, since I no longer run npm run lerna, it works for me 🙂.

@matuzalemsteles
Copy link
Member Author

Hey @jbalsas, I just updated with the latest release of Electric, it looks ready to go now.

@matuzalemsteles matuzalemsteles changed the title [DO NOT MERGE] Generate API Documents Generate API Documents Mar 16, 2018
@jbalsas
Copy link
Contributor

jbalsas commented Mar 19, 2018

Hey @matuzalemsteles, I'm getting the following error when trying to build the site:

[08:21:52] Finished 'metal:render:bundles' after 1.68 min
[08:23:43] Error when trying to render the "/Users/jose.balsas/dev/liferay/clay/packages/clayui.com/.temp/task/metal/site/pages/docs/components/charts/advanced/axis_label.js" file
[08:23:43] Details: Cannot read property 'call' of undefined
[08:23:43] Error when trying to render the "/Users/jose.balsas/dev/liferay/clay/packages/clayui.com/.temp/task/metal/site/pages/docs/components/charts/advanced/axis_range.js" file
[08:23:43] Details: Cannot read property 'call' of undefined
[08:23:43] Error when trying to render the "/Users/jose.balsas/dev/liferay/clay/packages/clayui.com/.temp/task/metal/site/pages/docs/components/charts/advanced/data_color.js" file
[08:23:43] Details: Cannot read property 'call' of undefined
[08:23:43] Error when trying to render the "/Users/jose.balsas/dev/liferay/clay/packages/clayui.com/.temp/task/metal/site/pages/docs/components/charts/advanced/grid_lines.js" file
[08:23:43] Details: Cannot read property 'call' of undefined
[08:23:43] Error when trying to render the "/Users/jose.balsas/dev/liferay/clay/packages/clayui.com/.temp/task/metal/site/pages/docs/components/charts/advanced/regions.js" file
[08:23:43] Details: Cannot read property 'call' of undefined
Warning: Error: Invalid state passed to 'axisX.tick.format'. Expected type 'function', but received type 'string'. Passed to 'Chart'. 
[08:23:43] Error when trying to render the "/Users/jose.balsas/dev/liferay/clay/packages/clayui.com/.temp/task/metal/site/pages/docs/components/charts/advanced/x_axis_tick_formatting.js" file
[08:23:43] Details: Cannot read property 'call' of undefined
[08:23:43] Error when trying to render the "/Users/jose.balsas/dev/liferay/clay/packages/clayui.com/.temp/task/metal/site/pages/docs/components/charts/basic/bar_chart.js" file
[08:23:43] Details: Cannot read property 'call' of undefined
[08:23:43] Error when trying to render the "/Users/jose.balsas/dev/liferay/clay/packages/clayui.com/.temp/task/metal/site/pages/docs/components/charts/basic/bubble_chart.js" file
[08:23:43] Details: Cannot read property 'call' of undefined
[08:23:43] Error when trying to render the "/Users/jose.balsas/dev/liferay/clay/packages/clayui.com/.temp/task/metal/site/pages/docs/components/charts/basic/combination_chart.js" file
[08:23:43] Details: Cannot read property 'call' of undefined
[08:23:43] Error when trying to render the "/Users/jose.balsas/dev/liferay/clay/packages/clayui.com/.temp/task/metal/site/pages/docs/components/charts/basic/donut_chart.js" file
[08:23:43] Details: Cannot read property 'call' of undefined
[08:23:43] Error when trying to render the "/Users/jose.balsas/dev/liferay/clay/packages/clayui.com/.temp/task/metal/site/pages/docs/components/charts/basic/gauge_chart.js" file
[08:23:43] Details: Cannot read property 'call' of undefined
[08:23:43] Error when trying to render the "/Users/jose.balsas/dev/liferay/clay/packages/clayui.com/.temp/task/metal/site/pages/docs/components/charts/basic/line_chart.js" file
[08:23:43] Details: Cannot read property 'call' of undefined
[08:23:43] Error when trying to render the "/Users/jose.balsas/dev/liferay/clay/packages/clayui.com/.temp/task/metal/site/pages/docs/components/charts/basic/pie_chart.js" file
[08:23:43] Details: Cannot read property 'call' of undefined
[08:23:43] Error when trying to render the "/Users/jose.balsas/dev/liferay/clay/packages/clayui.com/.temp/task/metal/site/pages/docs/components/charts/basic/scatter_chart.js" file
[08:23:43] Details: Cannot read property 'call' of undefined
[08:23:43] Error when trying to render the "/Users/jose.balsas/dev/liferay/clay/packages/clayui.com/.temp/task/metal/site/pages/docs/components/charts/basic/spline_chart.js" file
[08:23:43] Details: Cannot read property 'call' of undefined
[08:23:44] Error when trying to render the "/Users/jose.balsas/dev/liferay/clay/packages/clayui.com/.temp/task/metal/site/pages/docs/components/charts/basic/step_chart.js" file
[08:23:44] Details: Cannot read property 'call' of undefined
[08:23:44] Finished 'metal:render:html' after 3.55 min
[08:23:44] Finished 'metal' after 4.72 min
[08:23:44] Starting 'clean:temp'...
(node:58964) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): Error: EINVAL: invalid argument, rmdir '/Users/jose.balsas/dev/liferay/clay/packages/clayui.com/.temp/project/v2.0.0-rc.10/packages/clayui.com/src/pages/docs'
(node:58964) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
MacBook-Pro-3:clay jose.balsas$ 

Also... it took about 5 minutes to fail... is this missing anything?

@matuzalemsteles
Copy link
Member Author

matuzalemsteles commented Mar 19, 2018

@jbalsas, This error is because of issue #740, the first moment seemed to be an error coming from Electric, but I tried rendering other Clay components and seems to work fine then it can be a problem inside ClayCharts ... It has to be investigated.

On the delay this is real, Electric is taking too long to render the components in html and ends up being annoying, because every time we update something it always renders the API again and a few more long minutes to wait. Maybe I'm wrong and it could be because of the mistakes...

Edit: Soon after the error stopped the console?

@jbalsas
Copy link
Contributor

jbalsas commented Mar 19, 2018

So, what should I do to test this? Can we fix that? How are you testing it?

@matuzalemsteles
Copy link
Member Author

I'm just running npm run web to be able to test, it just does not render the pages of the charts, so I can see the API.

I am investigating this error at the moment, ClayGeomap it is rendered minus ClayCharts which is pretty strange, I even tested if it can be rendered in another component of Clay, but it works fine. In the error message, it does not help much.

@matuzalemsteles
Copy link
Member Author

matuzalemsteles commented Mar 19, 2018

Hey @jbalsas, now we can continue with this 🙂.

Electric calls the renderToString of the metal-component to render, renderToString of a dispose and calls the lifecycle disposed of Chart and soon after ChartBase with that we have super.disposed(), with this ends up causing the error. I'm not being able to go that far to find out why. Decide to withdraw, since we are not using. What do you think?

@carloslancha
Copy link
Contributor

Hey @matuzalemsteles For me all lifecycle inherited methods should be called. Maybe right now disposed() is doing nothing neither in DataComponent or metal's Component, but we cannot tell if in the future they will.

In fact I think is a good practice we should follow on every component.

Your thoughts @jbalsas ? :)

@jbalsas
Copy link
Contributor

jbalsas commented Mar 20, 2018

Yeah, all lifecyle should be called. There's likely something wrong in DataComponent::destroy. Could you figure out what it is in there, @matuzalemsteles?

@matuzalemsteles
Copy link
Member Author

Hey @jbalsas @carloslancha, That sounds good to me, I'll reverse this.

I've been analyzing, the ChartBase super had no view on DataComponent to use super.disposed(), the error up, is not quite clear since it was to launch something like:

TypeError: (intermediate value).disposed is not a function

As the webpack transpiles the super function, it calls the object and ends up throwing that error. If it looks something like this:

_get(_obj.__proto__ || Object.getPrototypeOf(_obj), 'disposed', this).call(this);

So I'm just adding ChartBase visibility to DataComponent. @jbalsas What do you think?

@jbalsas
Copy link
Contributor

jbalsas commented Mar 26, 2018

Hey @matuzalemsteles, why would this be needed in ChartBase? @julien, could you take a look?

@matuzalemsteles
Copy link
Member Author

Hey @jbalsas, Because Electric uses the renderToString to render the components, it's a dispose. When we use ClayCharts on the site, we can not render because of the super.disposed() that can not see the lifecyle of the Component in Metal or DataComponent. This solves the problem by giving visibility.

The same error is given when we give a dispose in any component of Charts.

@jbalsas
Copy link
Contributor

jbalsas commented Mar 26, 2018

Hey @matuzalemsteles, yeah, I can see that, but since it is extending DataComponent, we are likely doing something wrong when assigning the prototypes, so I'd like to understand why is that happening.

@julien, we need to add some tests to charts to at least test that the lifecycle of the components is properly accessible. Also, I'm not sure the jsx charts actually use DataComponent, so they are likely to be missing some stuff. Could you double-check?

@julien
Copy link
Contributor

julien commented Mar 27, 2018

@jbalsas, @matuzalemsteles sorry, I didn't receive any notification from GitHub yesterday which is why I didn't reply yet. I just fetched the GenerateAPIDocs branch and ran npm run web locally, it took some time, but this is the only error that showed in the console

Warning: Error: Invalid state passed to 'axisX.tick.format'. Expected type 'function', but received type 'string'. Passed to 'Chart'. 

Which is probably due to this
(GitHub renders markdown automatically, you'll have to look at line 51 since I don't know how to link to it directly)

@jbalsas Ok, I'll add some tests to check that everything works as expected with the component's lifecycle methods, and have a look at the jsx components.

@matuzalemsteles in the meantime, if you need to make any changes or need some help, feel free to ping me.

@julien
Copy link
Contributor

julien commented Mar 27, 2018

@jbalsas and @matuzalemsteles I removed super.disposed() in ChartBase.disposed(), since it had nothing to do there.

I also added various lifecycle tests for Charts, here.
I'll check the jsx versions too and let you know.

@matuzalemsteles
Copy link
Member Author

@julien Thanks for the answer.

Hey @jbalsas, I've been giving it a lookup, and Object.assign does not work as expected when we have a super in our methods, there's more information about it here.

On one hand, you can’t move a method that uses super: Such a method has the internal slot [[HomeObject]] that ties it to the object it was created in. If you move it via Object.assign(), it will continue to refer to the super-properties of the original object. Details are explained in a section in the chapter on classes.

@jbalsas
Copy link
Contributor

jbalsas commented Mar 28, 2018

Yeah, that makes sense... can you open an issue so we figure out how to better structure the charts hierarchy?

I'm trying this without the changes on top of develop and I think it should be good to merge soon! :)

@jbalsas jbalsas merged commit baf9040 into liferay:develop Mar 28, 2018
@jbalsas
Copy link
Contributor

jbalsas commented Mar 28, 2018

Works like a charm, @matuzalemsteles!! Nice job! 👍

@matuzalemsteles
Copy link
Member Author

Thanks. Okay, I can work on this later.

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.

5 participants