-
Notifications
You must be signed in to change notification settings - Fork 94
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
Make readme improvements #1279
base: main
Are you sure you want to change the base?
Make readme improvements #1279
Conversation
Hi @Giuulob89 thanks for the help! If this is still a work in progress, please mark the PR as draft. When it's no longer a draft, you can switch that off. When you do want it reviewed, please also ask for my review. Thanks a ton! |
Hi, @wwahammy! The changes are ready to be reviewed now! |
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.
I like this! It gives it a nicer, more professional look!
Other than the review comment, there are a few changes that I think would help improve this:
- Don't put the svg files inside the``app
folder. That makes them available for the application itself and we don't want that. Maybe put it under
docs/images`? addicional_documentation.svg
is misspelled. It should beadditional_documentation.svg
.- The Houdini logo is too big on the README. Additionally, it looks bad when in Github dark mode. I think, for now, it wouldn't hurt to just remove it.
Once, you have those, please request a new review and we should be ready to merge. Good work!
README.md
Outdated
@@ -27,40 +44,42 @@ comfort and speed. | |||
|
|||
All new backend code and React components should be well tested. | |||
|
|||
## Supported operating systems | |||
## ![Supported systems](./app/assets/images/ui_components/system_settings.svg) **Supported operating systems** |
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.
Is there anyway to not link to the SVG itself and still display it? If not, that's fine but it's kind of a weird experience when you click it. If you can avoid linking to it, please change it on all of the SVGs.
Hi @Giuulob89 @murilogds, is there any update on this? |
Hi @wwahammy we've been overwhelmed with other activities these last few weeks and we haven't been able to finish it yet, we'll try to do that this week. |
Co-authored-by: Murilo Gomes <[email protected]>
Co-authored-by: Murilo Gomes <[email protected]>
Co-authored-by: Murilo Gomes <[email protected]>
Co-authored-by: Murilo Gomes <[email protected]>
Co-authored-by: Murilo Gomes <[email protected]>
0f7151a
to
919cd40
Compare
Hi @Giuulob89 @murilogds: if this is ready to review, please make sure to request a review by pressing the little spinning arrow button next to my name in the list of reviewers (top right on the page) |
Hi @Giuulob89 @murilogds, just a check-in. Is this ready for review? |
Hi @Giuulob89 @murilogds, I wanted to check in here again. Is this ready for review? |
Hi, @wwahammy ! Sorry we didn't answered earlier! This is ready to review! Thanks |
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 the changes all!
I noticed that the svgs are still in the app directory. Please move them over to docs/images
Also, one tiny additional change, the please change the spelling of filename of addicional_documentation.svg
to additional_documentation.svg
.
Thanks a ton! With these super minor changes, I think we'll be able to merge.
We made some changes in the readme's face and put some notes to make commands more clear.
Closes issue: #1236
Co-authored-by: Murilo Gomes [email protected]