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.

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

feat: Adjust task and waker panel for long names #527

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

devanbenz
Copy link
Contributor

@devanbenz devanbenz commented Feb 17, 2024

close Friendly ping~ Do you have time to work on it again?

  • Makes better use of waker and task panel spacing
  • Puts Last woken on its own line
  • wraps text within the task panel for really long location names

Updated:
task_long_name
task_no_name

@devanbenz devanbenz requested a review from a team as a code owner February 17, 2024 18:18
Copy link
Collaborator

@hds hds left a comment

Choose a reason for hiding this comment

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

I haven't had a chance to look to much into the code yet. But from the screenshots, it looks like you're losing lines from the Task panel (I see the scheduled and idle times missing from the bottom).

I'm not sure if you've noticed this, but the task panel will need to have a variable height, whereas right now the height is fixed.

Also, I think that we probably don't need to put the location on its own line (after the label), it should be able to start on the line with the label and then break onto additional lines if it needs to.

I'll come back with a complete review (hopefully tomorrow) once I've had a chance to read the code changes.

@devanbenz
Copy link
Contributor Author

devanbenz commented Feb 18, 2024 via email

Copy link
Collaborator

@hds hds left a comment

Choose a reason for hiding this comment

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

A few more comments based on reading the changes this time.

@@ -9,6 +9,7 @@ use crate::{
help::HelpText,
},
};
use ratatui::widgets::Wrap;
Copy link
Collaborator

Choose a reason for hiding this comment

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

style: merge this with the other ratatui imports ok the next line.

@@ -131,8 +132,8 @@ impl TaskView {
.direction(layout::Direction::Horizontal)
.constraints(
[
layout::Constraint::Percentage(50),
layout::Constraint::Percentage(50),
layout::Constraint::Percentage(60),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we might want something more dynamic than this. What do you think?

@devanbenz
Copy link
Contributor Author

Will be coming back to this PR this weekend! Sorry I was out of town last week/over the weekend :D

@Rustin170506
Copy link
Collaborator

@devanbenz Friendly ping~ Do you have time to work on it again? Thanks!

@devanbenz
Copy link
Contributor Author

@devanbenz Friendly ping~ Do you have time to work on it again? Thanks!

I definitely want to get back to this. I have an interview this week and a talk during the weekend. I have a feeling that afterwards I will likely be able to start contributing to open source more 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants