Skip to content

Conversation

@heftymouse
Copy link
Contributor

Resolved / Related Issues

To prevent extra work, all changes to the Files codebase must link to an approved issue marked as Ready to build. Please insert the issue number following the hashtag with the issue number that this Pull Request resolves.

  • Closes #

Steps used to test these changes

Stability is a top priority for Files and all changes are required to go through testing before being merged into the repo. Please include a list of steps that you used to test this PR.

  1. Opened Files ...
  2. ...

@heftymouse heftymouse marked this pull request as draft June 5, 2024 22:33
@mdtauk
Copy link
Contributor

mdtauk commented Jun 5, 2024

Does the new speed graph match the design, with the gradient brush, the positioning of the Speed value with padding above the graph to avoid overlapping, and the stroke colour adapting to theme changes?

image

@heftymouse
Copy link
Contributor Author

Right now, not at all, I'll get that fixed soon

@yaira2 yaira2 added the ready for review Pull requests that are ready for review label Jun 10, 2024
@yaira2
Copy link
Member

yaira2 commented Jun 14, 2024

@heftymouse is this ready for review?

@heftymouse
Copy link
Contributor Author

I have to make it respond to theme changes and unhook all the event handlers, but most of it is done

@heftymouse heftymouse marked this pull request as ready for review June 15, 2024 15:31
@heftymouse heftymouse requested a review from ahmed605 June 15, 2024 15:31
<Grid
x:Name="MainGraphCartesianChartClipGrid"
Height="72"
Height="128"
Copy link
Member

Choose a reason for hiding this comment

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

Is this increased height by design?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put it there since it was easier to see in testing and I thought it looked nicer but it's not necessary

Copy link
Member

Choose a reason for hiding this comment

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

I know this was discussed on Discord, what was the conclusion?

Copy link
Member

Choose a reason for hiding this comment

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

Martin said 80 if 72 didn' look right.
And still 80 looks short to @heftymouse (me as well a bit).

Copy link
Contributor

Choose a reason for hiding this comment

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

72 is what the design uses, but the height should balance out the heading padding for the speed text and the chart height.

We are not trying to match File Explorer, and we should be conscious of how tall the flyout becomes with multiple operations on-going and the expanded size.

@yaira2 yaira2 changed the title Feature: Add Composition based speed graph Code Quality: Add Composition based speed graph Jun 16, 2024
Copy link
Member

@yaira2 yaira2 left a comment

Choose a reason for hiding this comment

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

LGTM

@yaira2 yaira2 merged commit 0997045 into files-community:main Jun 17, 2024
@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed ready for review Pull requests that are ready for review labels Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready to merge Pull requests that are approved and ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants