Skip to content

Conversation

@filzrev
Copy link
Collaborator

@filzrev filzrev commented Jul 4, 2023

This PR implement feature described #8927.

What's is added to docfx CLI command

Following options are added for docfx build and docfx serve commands.

--open-browser                Open a web browser when the hosted website starts
--open-browser-relative-path  Specify the relative path to open

Usage

To launch browser. pass --open-browser switch option to command.
It launch browser with root site URL (e.g. http:localhost:8080).

When --open-browser-relative-path optional parameter is passed.
It try to resolve output HTML file by using manifest.json file content.
If failed to resolve HTML path. then error log is recorded. and fallback to site root URL.

docfx build --serve
docfx build --serve --open-browser
docfx build --serve --open-browser --open-browser-relative-path docs/config.html
docfx build --serve --open-browser --open-browser-relative-path docs/config.md 
Launch with specific page.
docfx serve _site --open-browser
docfx serve _site --open-browser --open-browser-relative-path docs/config .html
docfx serve _site --open-browser --open-browser-relative-path docs/config .md

Unit Tests

This PR don't contains unit tests.
Because it's difficult to write unit tests that relating OS commands and launch browser.

Tests are executed manually that cover all execution paths.
Excepting macOS environment browser launch test.

This PR use same OS command(open) that is used by dotnet-serve to open browser.
I thought, there is no problems on macOS environment.

Documentation

This PR don't contains documentation updates.
Currently there are few references about docfx serve and docfx build --serve command in the documentation.

It might be better to update following related docs later.

Additional Design Notes

Composing Commands
This PR add options as simple options.
Spectre.Console.Cli supports Composing Commands.
It might be better to change --serve related options as Composing Commands.

Redundant argument names
--open-browser-relative-path is a bit redundant option name.
--open-browser:[url] style option is preferred that is implemented by the dotnet serve command.

But It seemsSpectre.Console.Cli can't handle this option styles.

@codecov
Copy link

codecov bot commented Jul 4, 2023

Codecov Report

Patch coverage: 1.88% and project coverage change: -0.15 ⚠️

Comparison is base (1263a81) 77.05% compared to head (92e97ed) 76.90%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8934      +/-   ##
==========================================
- Coverage   77.05%   76.90%   -0.15%     
==========================================
  Files         605      605              
  Lines       25042    25091      +49     
==========================================
+ Hits        19295    19297       +2     
- Misses       5747     5794      +47     
Impacted Files Coverage Δ
src/Microsoft.DocAsCode.App/RunServe.cs 0.00% <0.00%> (ø)
src/docfx/Models/BuildCommand.cs 40.38% <0.00%> (ø)
src/docfx/Models/DefaultCommand.cs 91.42% <0.00%> (ø)
src/docfx/Models/ServeCommand.cs 0.00% <0.00%> (ø)
src/docfx/Models/BuildCommandOptions.cs 86.95% <50.00%> (-3.52%) ⬇️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@yufeih
Copy link
Contributor

yufeih commented Jul 5, 2023

Is it simpler to have an --open-file option for opening a specific URL from source file that implies the --open-browser switch?

Opens the launch URL: docfx serve _site --open-browser
Opens URL from a file: docfx serve _site --open-file docs/config.md

@filzrev
Copy link
Collaborator Author

filzrev commented Jul 5, 2023

Is it simpler to have an --open-file option for opening a specific URL from source file that implies the --open-browser switch?
Opens the launch URL: docfx serve _site --open-browser
Opens URL from a file: docfx serve _site --open-file docs/config.md

In latest commit.
Change implementation as you noted.

And I founds a bug that using Path.Combine to url. It cause test fails on non windows environment.
I've fixed this problems and refactor codes to ensure fallback to baseUrl when failed to resolve relative path.

Copy link
Contributor

@yufeih yufeih left a comment

Choose a reason for hiding this comment

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

Looks great now! Thank you @filzrev !

@yufeih yufeih merged commit 38af992 into dotnet:main Jul 5, 2023
@yufeih yufeih added the new-feature Makes the pull request to appear in "New Features" section of the next release note label Jul 5, 2023
@filzrev filzrev deleted the add-open-browser-supports branch July 5, 2023 01:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

new-feature Makes the pull request to appear in "New Features" section of the next release note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants