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

Code Quality: Refactor Status Center code #13276

Merged
merged 49 commits into from
Sep 12, 2023

Conversation

0x5bfa
Copy link
Member

@0x5bfa 0x5bfa commented Aug 29, 2023

Feature: Display transfer speed in status center

Motivation & Context

  • Removed IStatusCenterAction interface used to access viewmodel because we now have Dependency injection.
  • Removed PostedStatusBanner class and merged it to StatusBanner class which was renamed to StatusCenterItem
  • Renamed some classes to use same words
  • Updated UI

Validation
How did you test these changes?

  • Did you build the app and test your changes?
  • Did you check for accessibility? You can use Accessibility Insights for this.
  • Did you remove any strings from the en-us resource file?
    • Did you search the solution to see if the string is still being used?
  • Did you implement any design changes to an existing feature?
    • Was this change approved?
  • Are there any other steps that were used to validate these changes?
    • Open Files
    • Copy/move/delete files
    • Open StatusCenter
    • You see what has been changed

PR TO-DO

  • Remove InfoBar
  • Add color visual states

Concepts

image

Screenshots

image

@mdtauk
Copy link
Contributor

mdtauk commented Aug 29, 2023

I know it matches the design I originally set out to do, but I would value some input to improve visibility of the Speed line, and see what options we have.

Other examples...
image
image

image
image

Something like this may be a bit cleaner
image

@yaira2
Copy link
Member

yaira2 commented Aug 29, 2023

@0x5bfa we haven't finalized any plans around this flyout.

@yaira2
Copy link
Member

yaira2 commented Aug 29, 2023

I'd like to get a speed graph in there sooner than later, but we need to spec this out.

@0x5bfa
Copy link
Member Author

0x5bfa commented Aug 29, 2023

I can wait. how long will it take?

I thought the spec was already formulated when you mentioned this concepts and I raised my hand.

@0x5bfa 0x5bfa changed the title Feature: Redesign StatusCenter Feature: Display transfer speed in status center Aug 29, 2023
@mdtauk
Copy link
Contributor

mdtauk commented Aug 29, 2023

I can wait. how long will it take?

I thought the spec was already formulated when you mentioned this concepts and I raised my hand.

The concepts were done like a year or two ago. There was going to be a task UI "Status centre" and it was at a different time.

I was not asked to look into it any further since, so I guess we can work on this aspect and maybe implement it later, when other bits are figured out.

  • Do we want this flyout UI to be always accessible?
  • Do we automatically remove completed tasks?
  • Do we want notifications when apps are in the background? (Adaptive Cards?)
  • Do we want the icon for this UI to reflect progress or number of tasks in operation?
  • How do we handle a polished transition between progress and graph?

@mdtauk
Copy link
Contributor

mdtauk commented Aug 30, 2023

This is what we are working towards right now

image

Hover to show the remove button on completed and failed actions - @yaira2 suggested for touch that this is handled with the SwipeControl

@mdtauk
Copy link
Contributor

mdtauk commented Aug 31, 2023

Here is a rough idea of how we could transition from a Progress control to a Progress Chart - how possible is something like this @0x5bfa

Replaced the video with one where the Progress Bar style before expansion, matches the system style.

status.card.expanding.chart.2.mp4

@yaira2 yaira2 requested a review from QuaintMako September 7, 2023 14:45
@0x5bfa
Copy link
Member Author

0x5bfa commented Sep 7, 2023

Ready for review for me.

@yaira2
Copy link
Member

yaira2 commented Sep 7, 2023

Are there any other steps that were used to validate these changes?

Can you share some testing steps?

@0x5bfa
Copy link
Member Author

0x5bfa commented Sep 7, 2023

omg i'm sorry i will add soon (i'm in bed so it will be tomorrow)

@yaira2
Copy link
Member

yaira2 commented Sep 7, 2023

It looks good code wise.

@0x5bfa
Copy link
Member Author

0x5bfa commented Sep 8, 2023

Done

@yaira2
Copy link
Member

yaira2 commented Sep 8, 2023

Open StatusCenter

Does this work for you? The app crashes when I try opening the status center.

@0x5bfa
Copy link
Member Author

0x5bfa commented Sep 8, 2023

Last time I tested successfully is 1a120fe

Copy link
Contributor

@QuaintMako QuaintMako left a comment

Choose a reason for hiding this comment

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

Great work! I don't have much to add aside of very small formatting ideas. Thanks for handling this big feature!

src/Files.App/UserControls/StatusCenter.xaml.cs Outdated Show resolved Hide resolved
src/Files.App/UserControls/StatusCenter.xaml.cs Outdated Show resolved Hide resolved
src/Files.App/UserControls/StatusCenter.xaml Show resolved Hide resolved
src/Files.App/Utils/StatusCenter/StatusCenterItem.cs Outdated Show resolved Hide resolved
src/Files.App/Utils/StatusCenter/StatusCenterItem.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@QuaintMako QuaintMako left a comment

Choose a reason for hiding this comment

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

LGTM code-wise, I am not able to test in-app.

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.

I ran some tests and it works as expected, nice work!

@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels Sep 12, 2023
@yaira2 yaira2 merged commit 938de3d into files-community:main Sep 12, 2023
2 checks passed
@0x5bfa 0x5bfa deleted the 5bfa/Cleanup-StatusCenter branch September 12, 2023 13:35
<Button
x:Name="SearchButton"
x:Name="ShowSearchButton"
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this was being used in a visual state, I'll open a PR to fix.

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.

4 participants