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

[Docs] Add developer documentation for using/modifying the chrome service #1875

Merged
merged 5 commits into from
Jul 26, 2022

Conversation

kaddy645
Copy link
Contributor

@kaddy645 kaddy645 commented Jul 11, 2022

Signed-off-by: kaddy645 xdeskart@amazon.com

Description

As a plugin or core dashboards developer, I'd like to have a README for the chrome service to be able to quickly and consistently integrate plugins with the global application chrome.

SC

Screen Shot 2022-07-11 at 10 57 38 AM

Issues Resolved

Issue-1857

Check List

  • New functionality includes testing.
    • All tests pass
      • yarn test:jest
      • yarn test:jest_integration
      • yarn test:ftr
  • New functionality has been documented.
  • Commits are signed per the DCO using --signoff

Signed-off-by: kaddy645 <xdeskart@amazon.com>
@kaddy645 kaddy645 requested a review from a team as a code owner July 11, 2022 17:53
@kaddy645 kaddy645 self-assigned this Jul 11, 2022
@kaddy645 kaddy645 changed the title Add developer documentation for using/modifying the chrome service [Docs] Add developer documentation for using/modifying the chrome service Jul 11, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jul 11, 2022

Codecov Report

Merging #1875 (2695e5b) into main (53e98a6) will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1875      +/-   ##
==========================================
- Coverage   67.51%   67.49%   -0.03%     
==========================================
  Files        3073     3076       +3     
  Lines       59118    59144      +26     
  Branches     8986     8989       +3     
==========================================
+ Hits        39914    39919       +5     
- Misses      17020    17041      +21     
  Partials     2184     2184              
Impacted Files Coverage Δ
...lication/angular/doc_table/components/table_row.ts 83.09% <0.00%> (-8.46%) ⬇️
src/plugins/discover/public/plugin.ts 1.86% <0.00%> (-0.24%) ⬇️
src/plugins/discover/public/get_inner_angular.ts 81.25% <0.00%> (ø)
...n/components/doc_viewer_links/doc_viewer_links.tsx 100.00% <0.00%> (ø)
...er/public/application/angular/doc_viewer_links.tsx 100.00% <0.00%> (ø)
...cation/doc_views_links/doc_views_links_registry.ts 0.00% <0.00%> (ø)
.../discover/public/opensearch_dashboards_services.ts 66.66% <0.00%> (+1.66%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 53e98a6...2695e5b. Read the comment docs.

Copy link
Member

@ashwin-pc ashwin-pc left a comment

Choose a reason for hiding this comment

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

Always lovely to see new docs!

Love the work put in to make it look presentable, but i think that the lack of standardization in markdown is kinda ruining it a little. Given that we dont know what viewer the user will use to read this file, lets make sure it semantically correct? I'm open to suggestions but imo i'd:

  • avoid using html in markdown
  • use ``` only for multiline code snippets
  • use code snippet blocks strictly for code (The code snippet should be copy paste-able)
  • make sure that the readme is easily readable without a markdown viewer too

As for the doc itself, the content is good. I'd only like to see more of an abstract of what the service is and does as a whole so that i can decide if its what i want.

src/core/public/chrome/README.md Outdated Show resolved Hide resolved
src/core/public/chrome/README.md Outdated Show resolved Hide resolved
src/core/public/chrome/README.md Show resolved Hide resolved
src/core/public/chrome/README.md Outdated Show resolved Hide resolved
src/core/public/chrome/README.md Outdated Show resolved Hide resolved
src/core/public/chrome/README.md Show resolved Hide resolved
src/core/public/chrome/README.md Outdated Show resolved Hide resolved
src/core/public/chrome/README.md Outdated Show resolved Hide resolved
Signed-off-by: kaddy645 <xdeskart@amazon.com>
@kaddy645 kaddy645 requested a review from ashwin-pc July 12, 2022 18:30
@CPTNB
Copy link
Contributor

CPTNB commented Jul 12, 2022

Thanks for writing this up! I see a few different types of content in this file and I'd like you to tease them into individual files and places.

At the highest level you have general guidance about what the plugin is, why it exists, how it works, and in vague terms how it's used. This is good stuff as an at-a-glance reference to future developers, and I think it's what Josh had in mind when he wrote the original issue.

Next you have more lower level API documentation, and I'm not so excited to see this kind of stuff in this type of document. Personally, I prefer my API docs to be very well structured so that I can quickly scan them to get the information I need. I don't want to scan through a narrative to fish out API info. I'm not sure hand-written API docs is the right way to go anyway, since that sort of thing is easily (at least, more easily) auto-generated and automatically kept up to date.

Last, you have some information about libraries and concepts like Observables. I do think this kind of thing has a place, but I'd like to see it live elsewhere from plugin-specific information.

My guidance would be to coalesce the high level information about the plugin into a few narrative paragraphs at the top of the document, then group the API docs into task driven examples that show code blocks that solve what you think might be common tasks. Then, move the information about libraries and Observables to the more free-form blog docs: https://github.com/CPTNB/opensearch-dashboards-dev-docs

@kaddy645
Copy link
Contributor Author

@CPTNB I agree with you. Observables and Load-ash should be somewhere else as that info is lang specific. I was just collecting right info so we can divide like you said. I'll remove it from this specific README for now.

Signed-off-by: kaddy645 <xdeskart@amazon.com>
@kaddy645 kaddy645 added the docs Improvements or additions to documentation label Jul 12, 2022
ashwin-pc
ashwin-pc previously approved these changes Jul 15, 2022
Copy link
Member

@ashwin-pc ashwin-pc left a comment

Choose a reason for hiding this comment

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

Amazing! thanks

src/core/public/chrome/README.md Outdated Show resolved Hide resolved
src/core/public/chrome/README.md Outdated Show resolved Hide resolved
src/core/public/chrome/README.md Show resolved Hide resolved
src/core/public/chrome/README.md Show resolved Hide resolved
src/core/public/chrome/README.md Outdated Show resolved Hide resolved
src/core/public/chrome/README.md Show resolved Hide resolved
Signed-off-by: kaddy645 <xdeskart@amazon.com>
Copy link
Member

@ananzh ananzh left a comment

Choose a reason for hiding this comment

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

NIT> some e.g left in the doc.

@kaddy645 kaddy645 merged commit 728f013 into opensearch-project:main Jul 26, 2022
@kaddy645 kaddy645 deleted the Issue-1857 branch July 26, 2022 16:57
@ananzh ananzh added the v2.2.0 label Jul 29, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jul 29, 2022
…vice (#1875)

* Add developer documentation for using/modifying the chrome service

Signed-off-by: kaddy645 <xdeskart@amazon.com>

* update: remove un-necessary code blocks

Signed-off-by: kaddy645 <xdeskart@amazon.com>

* remove js specific details

Signed-off-by: kaddy645 <xdeskart@amazon.com>

* pr updates

Signed-off-by: kaddy645 <xdeskart@amazon.com>
(cherry picked from commit 728f013)
kavilla pushed a commit that referenced this pull request Aug 1, 2022
…vice (#1875) (#2016)

* Add developer documentation for using/modifying the chrome service

Signed-off-by: kaddy645 <xdeskart@amazon.com>

* update: remove un-necessary code blocks

Signed-off-by: kaddy645 <xdeskart@amazon.com>

* remove js specific details

Signed-off-by: kaddy645 <xdeskart@amazon.com>

* pr updates

Signed-off-by: kaddy645 <xdeskart@amazon.com>
(cherry picked from commit 728f013)

Co-authored-by: Kartik Desai <xdeskart@amazon.com>
CPTNB pushed a commit to CPTNB/OpenSearch-Dashboards that referenced this pull request Aug 8, 2022
…vice (opensearch-project#1875)

* Add developer documentation for using/modifying the chrome service

Signed-off-by: kaddy645 <xdeskart@amazon.com>

* update: remove un-necessary code blocks

Signed-off-by: kaddy645 <xdeskart@amazon.com>

* remove js specific details

Signed-off-by: kaddy645 <xdeskart@amazon.com>

* pr updates

Signed-off-by: kaddy645 <xdeskart@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x docs Improvements or additions to documentation v2.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Docs] Add developer documentation for using/modifying the chrome service
5 participants