-
Notifications
You must be signed in to change notification settings - Fork 350
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
PF4: fix(README.md): provides URL location of workspace #3254
Conversation
PatternFly-React preview: https://patternfly-react-pr-3254.surge.sh |
Codecov Report
@@ Coverage Diff @@
## master #3254 +/- ##
=======================================
Coverage 67.43% 67.43%
=======================================
Files 892 892
Lines 24869 24869
Branches 2140 2140
=======================================
Hits 16770 16770
Misses 7094 7094
Partials 1005 1005
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this! I left a couple of questions/suggestions.
@seanforyou23 Thank you for your helpful feedback Michael! I've updated the README according to your suggestions. |
|
||
```sh | ||
yarn start:pf4 | ||
``` | ||
|
||
Go to localhost:8000. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although adding more info to the README is a good thing, this does not appear to address the original issue? That is, the yarn start:dev
command no longer confirms exactly where the app is running.
This also does not account for the example app running on a different port, should port 8000 already be occupied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dlabrecq Thank you for your feedback Dan! This has to do with something Zack addressed here: #3154 (comment) and I added that line based on what was agreed upon in this comment. But I see what you mean -- this PR won't necessarily close that issue.
Good point about the different port -- do you happen to know what port the app runs on if localhost:8000 is already occupied?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jenny-s51 if the port is already in use it prints to the console @patternfly/react-docs: Something is already running at port 8000
. I think it used to be the case that an interactive console would take over and allow you to specify a different port, then proceed to launch the dev server using that new config. It seems to have regressed or been removed at some point.
I'm personally ok with pulling these changes in, but I agree it doesn't really fix the root cause. So, as long as we can leave the related issue open for future fixes we should be ok. @dlabrecq does this sound ok to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, can leave that issue opened
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Your changes have been released in:
Thanks for your contribution! 🎉 |
Temporary fix for #3154 (comment)