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

fix: print error message when terminal size is less the number of lines #28

Merged
merged 12 commits into from
Apr 25, 2022
Merged

fix: print error message when terminal size is less the number of lines #28

merged 12 commits into from
Apr 25, 2022

Conversation

notjedi
Copy link
Contributor

@notjedi notjedi commented Apr 14, 2022 •
edited
Loading

fixes #19

this is my first time writing rust code, so please let me know if there is a better way of doing this.

Samyak2
Samyak2 requested changes Apr 15, 2022
View reviewed changes
Copy link
Owner

@Samyak2 Samyak2 left a comment

Choose a reason for hiding this comment

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

Thanks!

I had some comments.

Also, I was concerned about another thing. The size could be restricted by width too - some tiling WMs can make windows be very long and thin. We should also check to see if the longest line fits on the terminal.

src/main.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/tui.rs Outdated Show resolved Hide resolved
@Samyak2 Samyak2 added the enhancement New feature or request label Apr 15, 2022
@Samyak2
Copy link
Owner

Samyak2 commented Apr 15, 2022

Also, could you link the issue by adding "Fixes #19" in your PR's description?

@Samyak2 Samyak2 mentioned this pull request Apr 15, 2022
Closed
Samyak2
Samyak2 reviewed Apr 17, 2022
View reviewed changes
Copy link
Owner

@Samyak2 Samyak2 left a comment

Choose a reason for hiding this comment

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

This is looking good!

I had a few minor suggestions and a comment though.

src/main.rs Show resolved Hide resolved
src/tui.rs Outdated Show resolved Hide resolved
src/tui.rs Outdated Show resolved Hide resolved
src/tui.rs Show resolved Hide resolved
src/tui.rs Outdated Show resolved Hide resolved
@notjedi
Copy link
Contributor Author

notjedi commented Apr 18, 2022

i pushed a new commit with all the changes, do take a final look at it. also i appreciate you being patient and guiding me on writing rust for the first time. thank you.

Samyak2
Samyak2 approved these changes Apr 20, 2022
View reviewed changes
Copy link
Owner

@Samyak2 Samyak2 left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

@Samyak2
Copy link
Owner

Samyak2 commented Apr 20, 2022

The formatting step is failing. Could you run cargo fmt and fix it?

@notjedi
Copy link
Contributor Author

notjedi commented Apr 20, 2022

done @Samyak2

Samyak2
Samyak2 reviewed Apr 20, 2022
View reviewed changes
src/tui.rs Outdated Show resolved Hide resolved
Samyak2
Samyak2 requested changes Apr 20, 2022
View reviewed changes
src/tui.rs Outdated Show resolved Hide resolved
src/tui.rs Show resolved Hide resolved
Samyak2
Samyak2 requested changes Apr 20, 2022
View reviewed changes
Copy link
Owner

@Samyak2 Samyak2 left a comment

Choose a reason for hiding this comment

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

One more thing. Sorry for requesting so many changes xD, but I want to get this right.

src/tui.rs Show resolved Hide resolved
Samyak2
Samyak2 reviewed Apr 22, 2022
View reviewed changes
src/tui.rs Outdated Show resolved Hide resolved
Samyak2
Samyak2 approved these changes Apr 25, 2022
View reviewed changes
Copy link
Owner

@Samyak2 Samyak2 left a comment

Choose a reason for hiding this comment

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

LGTM.

Thank you for contributing and for your patience! 🎉

@Samyak2 Samyak2 merged commit c11b27a into Samyak2:main Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Samyak2 Samyak2 Samyak2 approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Show error if terminal size is too small
2 participants
@notjedi @Samyak2