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

Elyra Canvas Documentation using mkdocs #1702

Merged
merged 26 commits into from
Mar 1, 2024

Conversation

srikant-ch5
Copy link
Contributor

@srikant-ch5 srikant-ch5 commented Feb 19, 2024

Fixes: #1701

Developer's Certificate of Origin 1.1

   By making a contribution to this project, I certify that:

   (a) The contribution was created in whole or in part by me and I
       have the right to submit it under the Apache License 2.0; or

   (b) The contribution is based upon previous work that, to the best
       of my knowledge, is covered under an appropriate open source
       license and I have the right under that license to submit that
       work with modifications, whether created in whole or in part
       by me, under the same open source license (unless I am
       permitted to submit under a different license), as indicated
       in the file; or

   (c) The contribution was provided directly to me by some other
       person who certified (a), (b) or (c) and I have not modified
       it.

   (d) I understand and agree that this project and the contribution
       are public and that a record of the contribution (including all
       personal information I submit with it, including my sign-off) is
       maintained indefinitely and may be redistributed consistent with
       this project or the open source license(s) involved.

@srikant-ch5
Copy link
Contributor Author

Hi @matthoward366 Please review above setup for new Elyra Canvas Documentation

@matthoward366
Copy link
Member

@srikant-ch5 Is there a way I can test this out locally?

@srikant-ch5
Copy link
Contributor Author

@srikant-ch5 Is there a way I can test this out locally?

Hi @matthoward366 Yes you can run this using command mkdocs serve .

@tomlyn
Copy link
Member

tomlyn commented Feb 20, 2024

@matthoward366
For getting the docs build to work @Paul-Kawalkowski helped me discover that:

  1. Python v3 needs to be available. I had v3.11.3 installed
  2. Then run pip3 install -r requirements.txt
  3. Then run mkdocs serve
  4. When complete, open the browser: http://127.0.0.1:8000/

@srikant-ch5 Can you add this info to the Read me?

@srikant-ch5
Copy link
Contributor Author

@matthoward366 For getting the docs build to work @Paul-Kawalkowski helped me discover that:

  1. Python v3 needs to be available. I had v3.11.3 installed
  2. Then run pip3 install -r requirements.txt
  3. Then run mkdocs serve
  4. When complete, open the browser: http://127.0.0.1:8000/

@srikant-ch5 Can you add this info to the Read me?

Hi @tomlyn . Sure, I have added above instructions in Readme.

mkdocs.yml Outdated
edit_uri: edit/main/guide/

# Documentation and theme
copyright: 'Copyright &copy; 2023 IBM Watson Team, an open source project and all are welcome to contribute!<br> Maintained by the SPSS Modeler Team'
Copy link
Member

Choose a reason for hiding this comment

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

Update year to 2024. Maintained by the elyra canvas Team.

mkdocs.yml Outdated
@@ -0,0 +1,80 @@
site_name: Elyra Guide
Copy link
Member

Choose a reason for hiding this comment

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

This should be Elyra Canvas Guide

README.md Show resolved Hide resolved

### 3rd party styling

If you are using the expression builder in common-properties then also include these libraries:
Copy link
Member

Choose a reason for hiding this comment

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

Remove lines 44-47 -

If you are using the expression builder in common-properties then also include these libraries:

codemirror/addon/hint/show-hint.css
codemirror/lib/codemirror.css

@srikant-ch5 srikant-ch5 marked this pull request as draft February 22, 2024 06:33
@srikant-ch5
Copy link
Contributor Author

Hi @nmgokhale Thanks for the review. I have updated the PR with above feedback.

mkdocs.yml Outdated
edit_uri: edit/main/guide/

# Documentation and theme
copyright: 'Copyright &copy; 2024. Maintained by the Elyra Canvas Team., an open source project and all are welcome to contribute!<br> Maintained by the SPSS Modeler Team'
Copy link
Member

Choose a reason for hiding this comment

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

Please remove Maintained by the SPSS Modeler Team

tomlyn
tomlyn previously approved these changes Feb 22, 2024
guide/index.md Outdated
@@ -0,0 +1,90 @@
# Welcome to the Elyra Canvas Wiki
Copy link
Member

Choose a reason for hiding this comment

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

Change to Welcome to the Elyra Canvas Documentation

mkdocs.yml Outdated
edit_uri: edit/main/guide/

# Documentation and theme
copyright: 'Copyright &copy; 2023 IBM Watson Team, an open source project and all are welcome to contribute!<br> Maintained by the SPSS Modeler Team'
Copy link
Member

Choose a reason for hiding this comment

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

Should be
copyright: 'Copyright © 2024 Elyra Authors, an open source project and all are welcome to contribute!
Maintained by the Elyra Authors Team'

mkdocs.yml Outdated
@@ -0,0 +1,80 @@
site_name: Elyra Guide
Copy link
Member

Choose a reason for hiding this comment

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

Should be Elyra Canvas Documentation

guide/index.md Outdated

## Installation

You'll need to build your application with common canvas.
Copy link
Member

Choose a reason for hiding this comment

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

Should be:
You'll need to build your application with Elyra Canvas.

guide/index.md Outdated

You'll need to build your application with common canvas.

* common-canvas requires react, react-dom, react-intl, and react-redux libraries to be installed. See peerDependencies in package.json for versions requirements.
Copy link
Member

Choose a reason for hiding this comment

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

First sentence should be:
Elyra Canvas requires react, react-dom, react-intl, and react-redux libraries to be installed.

@@ -0,0 +1,3 @@
## Tips
- All input elements need to have a visual indication `focus` is set. This is different then selection.
- Use the correct html element for inputs. For example don't use a `div` with a click event. Use a `button` or `a` instead.
Copy link
Member

Choose a reason for hiding this comment

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

Please delete this document. It doesn't add anything useful to the documentation.

@srikant-ch5
Copy link
Contributor Author

Hi Team, I have deployed the docs at https://srikant-ch5.github.io/canvas/ Please review. The url can be changed to elyra-canvas .

@tomlyn tomlyn self-requested a review February 26, 2024 19:28
tomlyn
tomlyn previously approved these changes Feb 26, 2024
## Parameter Controls
The following controls are supported in the Common Properties editor. Control types are intended for use with particular parameter types:

### Control Types
Copy link
Member

Choose a reason for hiding this comment

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

slider control documentation is not added in this section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

slider control documentation is not added in this section.

Hi @nmgokhale I have included slider control documentation now https://srikant-ch5.github.io/canvas/3.3-Common-Properties-Controls/#control-types . Previously I was taking https://github.com/elyra-ai/canvas/wiki as reference (slider is not included in this).

nmgokhale
nmgokhale previously approved these changes Feb 27, 2024
on:
push:
branches:
- new-guide
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem right. Would this mean the guide would only be updated when the branch is new-guide?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes Matt, this needs to be changed as main when merged, for testing purpose I have kept it as new-guide.

- uses: actions/checkout@v4
- name: Configure Git Credentials
run: |
git config user.name github-actions[bot]
Copy link
Member

Choose a reason for hiding this comment

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

How did you come up with these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Matt to deploy the docs using gh-pages I have followed https://squidfunk.github.io/mkdocs-material/publishing-your-site/ resource and able to deploy whenever I push any change in docs to my new-guide branch. It triggers ci to deploy the changes without manually running mkdocs gh-deploy --force
Screenshot 2024-02-29 at 7 12 06 PM

Copy link
Member

Choose a reason for hiding this comment

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

I'm more wondering about git config user.name github-actions[bot] and git config user.email 41898282+github-actions[bot]@users.noreply.github.com. Do we need these?

@@ -73,7 +73,7 @@ const portsHorizontalDefaultLayout = {
labelAllowReturnKey: false, // true allows line feed to be inserted into label, "save" to make the return key save the label.

// An array of decorations to be applied to the node. For details see:
// https://github.com/elyra-ai/canvas/wiki/2.4.2-Decoration-Specification
// https://github.com/elyra-ai/canvas/tree/main/guide/2.4.2-Decoration-Specification.md
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't all these links be github pages links? Also, I don't this guide is the folder anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Matt, I have reverted the docs links to point to existing elyra canvas docs once the new one is deployed I will change these urls pointing to new docs.

@@ -488,7 +488,7 @@ const portsVerticalDefaultLayout = {
labelAllowReturnKey: false, // true allows line feed to be inserted into label, "save" to make the return key save the label.

// An array of decorations to be applied to the node. For details see:
// https://github.com/elyra-ai/canvas/wiki/2.4.2-Decoration-Specification
// https://github.com/elyra-ai/canvas/tree/main/guide/2.4.2-Decoration-Specification.md
Copy link
Member

Choose a reason for hiding this comment

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

Same as above. Have a look at all the links.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Matt, I have reverted the docs links to point to existing elyra canvas docs once the new one is deployed I will change these urls pointing to new docs.

@@ -0,0 +1,7 @@
th {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't all these new css and js files have copyrights?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Matt I have included copyrights in the new js and css files.

@@ -0,0 +1 @@
Images for the guide go here.
Copy link
Member

Choose a reason for hiding this comment

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

What is this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Matt I have removed this file now. Thanks.

// pages within the documentation will remain in the same tab
// Source: https://stackoverflow.com/questions/4425198/can-i-create-links-with-target-blank-in-markdown#answer-4425214

var links = document.links;
Copy link
Member

Choose a reason for hiding this comment

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

Missing copyright. Also, we usually use const instead of var

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I have updated these.

Copy link
Member

@matthoward366 matthoward366 left a comment

Choose a reason for hiding this comment

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

One small comment and the rest looks good. Once it's deployed with your branch then we can change it to main before merging.

- uses: actions/checkout@v4
- name: Configure Git Credentials
run: |
git config user.name github-actions[bot]
Copy link
Member

Choose a reason for hiding this comment

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

I'm more wondering about git config user.name github-actions[bot] and git config user.email 41898282+github-actions[bot]@users.noreply.github.com. Do we need these?

@srikant-ch5
Copy link
Contributor Author

I'm more wondering about git config user.name github-actions[bot] and git config user.email 41898282+github-actions[bot]@users.noreply.github.com. Do we need these?

Hi @matthoward366 I have removed these and its working fine. Thanks.

@matthoward366 matthoward366 marked this pull request as ready for review March 1, 2024 14:58
@matthoward366 matthoward366 merged commit 7a31f82 into elyra-ai:main Mar 1, 2024
3 checks passed
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.

Elyra Canvas New Documentation
5 participants