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.

Sign up for GitHub

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

Jump to bottom

Improve formatting ux #49

Merged
merged 4 commits into from
Oct 27, 2024
Merged

Improve formatting ux #49

merged 4 commits into from
Oct 27, 2024

Conversation

Erfanm83
Copy link
Contributor

Hi @dnnrly,

As part of Hacktoberfest, I've made the following improvements based on the discussion in issue #40 (Additional formatting):

  • Added borders to the description section to enhance readability.
  • Changed the status and summary section to a status bar using the lipgloss package.
  • Fixed failing tests caused by the UX changes.

Please let me know if there's any additional feedback or suggestions.

#40
Thanks,
@Erfanm83

dnnrly
dnnrly approved these changes Oct 25, 2024
View reviewed changes
httpref_test.go Outdated
Comment on lines 79 to 80
if actual != expected {
t.Errorf("Expected %v, but got %v", expected, actual)
Copy link
Owner

Choose a reason for hiding this comment

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

Can we use assert.Equal here?

view.go Outdated
Comment on lines 63 to 65
// if background color intended
// backgroundColor := "#353533"
// glamour.DarkStyleConfig.Document.BackgroundColor = &backgroundColor
Copy link
Owner

Choose a reason for hiding this comment

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

Please can this commented out code be removed?

@dnnrly
Copy link
Owner

dnnrly commented Oct 25, 2024

This is a great addition to httpref! Only some small amendments requested but otherwise a solid contribution. I might raise an issue with some additional work - but feel free to boost your Hacktoberfest PRs if you're up for it.

dnnrly
dnnrly requested changes Oct 25, 2024
View reviewed changes
Copy link
Owner

@dnnrly dnnrly left a comment

Choose a reason for hiding this comment

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

Small changes required.

Erfanm83
Erfanm83 commented Oct 27, 2024
View reviewed changes
Copy link
Contributor Author

@Erfanm83 Erfanm83 left a comment

Choose a reason for hiding this comment

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

Hi I have updated the code based on your reviews
Thanks for reveiwing if you had more of that let me know

@dnnrly dnnrly merged commit 66b47ac into dnnrly:master Oct 27, 2024
2 checks passed
@dnnrly
Copy link
Owner

dnnrly commented Oct 27, 2024

Thank you for your contribution! As is traditional for me, please let me know you Twitter username (if you have one) for me to start following you.

@Erfanm83
Copy link
Contributor Author

Thanks @dnnrly for meging my updates!
I will be so glad if you want more changes to this project
It looks like there is an issue of panic usage throughout the code, I want to fix that bug anyway

@Erfanm83 Erfanm83 mentioned this pull request Oct 27, 2024
Merged
@Erfanm83
Copy link
Contributor Author

Thank you for your contribution! As is traditional for me, please let me know you Twitter username (if you have one) for me to start following you.

No I hadn't Twitter(X) Account . But I Created one with this username : @Erfanm_2004
I Will add my Twitter Profile on my Github Account soon .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dnnrly dnnrly dnnrly approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Erfanm83 @dnnrly