Skip to content

[Property Editor] Emphasize properties the user has set in their code #8637

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

Merged
merged 18 commits into from
Jan 16, 2025

Conversation

elliette
Copy link
Member

@elliette elliette commented Dec 16, 2024

Fixes #8653
Work towards #1948

We would like to highlight the properties that the user has set in their code so that its visually clear what they have set.

I swapped the required label with a set label, and moved required to the input indicator (following the Material 3 guidance):

Screenshot 2025-01-14 at 2 34 17 PM

Screenshots of various themes (NOTE: before required label was changed):
Screenshot 2024-12-17 at 5 28 14 PM
Screenshot 2024-12-17 at 5 27 56 PM
Screenshot 2024-12-17 at 5 27 34 PM
Screenshot 2024-12-17 at 5 27 15 PM

Screenshot 2024-12-17 at 5 26 58 PM

@elliette elliette requested review from kenzieschmoll and a team as code owners December 16, 2024 18:44
@kenzieschmoll
Copy link
Member

This makes the non-set properties look disabled IMO. Is this something we could put on our list to get UX input on? what about another badge "explicit" or "set" instead of graying out some of the properties?

@elliette
Copy link
Member Author

This makes the non-set properties look disabled IMO. Is this something we could put on our list to get UX input on? what about another badge "explicit" or "set" instead of graying out some of the properties?

We already have 2 potential badges (isDefault and isRequired), and I think we will need to change the UI to fit 3.

I've tweaked how we are determining the background color, and added a bunch of different screenshots for various VS Code themes. Let me know what you think - mostly I want this in a nice-ish state for the demo, but I agree this is something we should follow up on with UX.

Note: I haven't added comments and tests yet, will add those if we think we should go ahead with these colors for now.

@kenzieschmoll
Copy link
Member

What about something more subtle like this?
Screenshot 2024-12-17 at 3 23 41 PM

@kenzieschmoll
Copy link
Member

Another affordance to consider if we want to reduce the number of badges like "default". We could use an asterisk * to indicate required parameters. Kind of like you'd expect to see in a form for required fields. Then maybe that would open up more space for a "set" or "explicit" badge if we think that would be more clear than using color.

@elliette
Copy link
Member Author

What about something more subtle like this? Screenshot 2024-12-17 at 3 23 41 PM

Oh I like this option! Changed to that and updated the screenshots.

@@ -7,7 +7,6 @@ import 'package:flutter/material.dart';
import 'package:flutter/services.dart';

import '../../../service/editor/api_classes.dart';
import '../../../shared/primitives/utils.dart';
import 'property_editor_controller.dart';

class PropertyEditorView extends StatelessWidget {
Copy link
Member

Choose a reason for hiding this comment

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

this widget doesn't provide much value to abstract now. Can we add the _PropertiesList directly where PropertyEditorView is currently used? That or make PropertyEditorView what _PropertiesList currently is (effectively deleting _PropertiesList and having PropertyEditorView contain the body of the current _PropertiesList)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we can! Though I'm planning on adding more to the PropertyEditorView soon (for example, the widget name) so will probably end up adding it back in later. Do you still think I should delete it for now?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can add a TODO for the parts that are coming soon

),
child: Row(
crossAxisAlignment: CrossAxisAlignment.start,
children: [
Copy link
Member

Choose a reason for hiding this comment

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

nit: it might be simpler to just add a Container to this row directly:

Row(
  children: [
    Container(
      width: _hasArgIndicatorWidth,
      color: argument.hasArgument ? theme.colorScheme.primary : Colors.transparent,
    ),
    ...
  ],
),

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm I tried that and was struggling to get it the proper height which is why I switched to border instead

@elliette elliette added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 16, 2025
Copy link

auto-submit bot commented Jan 16, 2025

auto label is removed for flutter/devtools/8637, due to - The status or check suite main has failed. Please fix the issues identified (or deflake) before re-applying this label.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jan 16, 2025
@elliette elliette merged commit 8046637 into flutter:master Jan 16, 2025
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use an asterix for required fields in the property editor
2 participants