Skip to content

sample creation banner #3113

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 3 commits into from
Jan 29, 2019
Merged

sample creation banner #3113

merged 3 commits into from
Jan 29, 2019

Conversation

pq
Copy link
Contributor

@pq pq commented Jan 23, 2019

Adds a sample creation banner to framework files w/ samples.

In the common (and current case), presents a simple label for the active sample.

single_gif

In case there is more than one sample (promised for the future), provides a chooser.

combo_gif

(Note this example above is showing more IDs than are appropriate for this file in order to demo the chooser behavior.)

NOTE: not for landing in M32 since we'll need to shore up the implementation in Android Studio.

/cc @stevemessick

Fixes: #2976.

@pq pq added this to the M33 milestone Jan 23, 2019
@stevemessick
Copy link
Member

@pq This is pretty cool! I want to patch it in and play around with it before finishing my review. Can I assume you're in no hurry since it is tagged for M33?

@pq
Copy link
Contributor Author

pq commented Jan 24, 2019

Thanks @stevemessick. No rush. Definitely an M33 feature.

Copy link
Member

@stevemessick stevemessick left a comment

Choose a reason for hiding this comment

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

This looks really good. I'd like to see a couple changes to the UI (layout, label) and maybe talk about a change to the UX (recreate vs reuse), but mostly it's good.

// Combo or label.
JComponent sampleSelector;

FlutterSampleActionsPanel(@NotNull List<FlutterSample> samples, @NotNull Project project) {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we can tighten up the layout of this panel? It takes a lot more vertical space than others and I didn't see an obvious need for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally. Trimmed by 5px:

image

We can tweak more too.

myLabel.setText("Open sample project:");
myLabel.setBorder(JBUI.Borders.emptyRight(5));

goLink = new JBLabel("<html><a href=\"\">Go...</a></html>");
Copy link
Member

Choose a reason for hiding this comment

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

"Go..." might be a bit too terse. Maybe "Open"? There is no user input so no need for ellipsis.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SGTM. @InMatrix and I went back and forth on this but I'm happy either way.

@InMatrix: any strong opinion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmmm...

image

Looks a little off?

Thoughts welcome!

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, "Open" twice is redundant. But the label could be changed to removed "Open". As a former Smalltalker, I'm obliged to suggest "Do It" (in jest).

Copy link
Member

@stevemessick stevemessick left a comment

Choose a reason for hiding this comment

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

Up to you what to do about the Open/Go label. :)

@pq
Copy link
Contributor Author

pq commented Jan 29, 2019

Thanks! I plan to do another pass after some feedback from @InMatrix so I'll leave it as is.

As for "do it", that sure does bring me back

image

sigh

Would that this were Smalltalk! 😢

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

Successfully merging this pull request may close these issues.

3 participants