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

Server > Get Started: use tested sources for DartPad Hello World #2092

Merged
merged 1 commit into from
Nov 14, 2019

Conversation

chalin
Copy link
Contributor

@chalin chalin commented Nov 14, 2019

Contributes to #1803 by replacing a Gist-based DartPad with a DartPad initialized from an analyzed and tested code excerpt from our examples folder.

Notes:

  • This PR uses the experimental DartPad inject_embed feature.
  • The DartPad is initialized with the base Hello World excerpt that was already a part of our examples, rather than create a new Hello World variant that illustrates use of const.

Followup to #1921
Related to #1919

Staged

https://dart-dev-staging-1.firebaseapp.com/tutorials/server/get-started

Screen shot

Screen Shot 2019-11-14 at 12 12 35

cc @johnpryan @legalcodes @RedBrogdon

@chalin chalin added fix.examples Adds or changes example test.general Relates to unit, integration, perf testing labels Nov 14, 2019
@chalin chalin requested a review from kwalrath November 14, 2019 17:47
@googlebot googlebot added the cla: yes Contributor has signed the Contributor License Agreement label Nov 14, 2019
Comment on lines -298 to -302

/* Hide "Open Dartpad" link */
.run-in-dartpad {
display: none;
}
Copy link
Contributor Author

@chalin chalin Nov 14, 2019

Choose a reason for hiding this comment

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

This CSS class was introduced July 11, 2016, Migrating repo from Ocupop. AFAICT, since the Dash migration, this style isn't used anymore (I've checked the site sources and the generated site).

Comment on lines -631 to -643

/* -----------------------------------------
Dartpad
----------------------------------------- */

#run-dartpad {
position: relative;
padding: 15px 0;
background: $gray-light;
border-top: 1px solid $gray;
z-index: 5;
}

Copy link
Contributor Author

@chalin chalin Nov 14, 2019

Choose a reason for hiding this comment

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

This CSS class was introduced July 11, 2016, Migrating repo from Ocupop. AFAICT, since the Dash migration, this style isn't used anymore (I've checked the site sources and the generated site).

Comment on lines -818 to -862
.codesample__open-in-dartpad {
display: block;
position: absolute;
width: auto;
z-index: 10;
padding: 5px 10px 5px 35px;
top: 35px;
right: 38px;
background-color: $gray;
color: $gray-base;

font-weight: normal;
font-size: $font-size-base * 0.91;
@include media-breakpoint-up(md) {
font-size: $font-size-base * 0.8;
}
@include media-breakpoint-up(lg) {
font-size: $font-size-base * 0.91;
}

&.top {
top: 0;
}
&:visited, &:active {
color: $gray-base;
}
&:hover {
background-color: darken($gray, 10%);
color: $white-base;
}
&:after {
content: '';
display: block;
position: absolute;
top: 5px;
left: 10px;
width: 18px;
height: 18px;
background-image: asset_url('shared/dart/logo/default.svg');
background-size: cover;
background-position: center center;
background-repeat: no-repeat;
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This CSS class was introduced July 11, 2016, Migrating repo from Ocupop. AFAICT, since the Dash migration, this style isn't used anymore (I've checked the site sources and the generated site).

Comment on lines +33 to +39
<style>
iframe[src^="https://dartpad"] {
border: 1px solid #ccc;
margin-bottom: 1rem;
width: 100%;
}
</style>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we like this approach (of initializing DartPads from code excerpts rather than Gists), then I'll move this style definition into an appropriate SCSS file.

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM

</style>

<?code-excerpt "misc/test/samples_test.dart (hello-world)"?>
```dart:run-dartpad
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@johnpryan, FYI, I'm keeping the language as dart because it allows IDEs etc to continue pretty-printing the code as Dart code:

It seems that inject_embed.dart is ok with class="language-dart:run-dartpad" instead of class="language-run-dartpad", as is document in https://github.com/dart-lang/dart-pad/wiki/Embedding-Guide#converting-code-blocks-to-dartpad. Maybe that wiki page entry could be updated?

</style>

<?code-excerpt "misc/test/samples_test.dart (hello-world)"?>
```dart:run-dartpad
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kwalrath - I switched to using the default mode-dart rather than mode-inline because the default mode look slightly better on desktop and looks no worse on mobile.

iframe[src^="https://dartpad"] {
border: 1px solid #ccc;
margin-bottom: 1rem;
width: 100%;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would vote to make this just a little bit taller (maybe 200px-400px) to encourage people try things out. We are adding a feature very soon to support this, but right now we might want to set a min-height

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What would be really nice would be to have a "grow frame" button at the bottom-right, just like GitHub comment boxes have:

Screen Shot 2019-11-14 at 13 06 44

WDYT? (Is there an issue / feature request open for this already? If not, would you like for me to open one?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since you've both approved this PR, I'll merge it as is. I'll address the min height in a followup PR.

@kwalrath - what are your feelings about the DartPad height for this sample?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to increase the height.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #2097

Comment on lines +33 to +39
<style>
iframe[src^="https://dartpad"] {
border: 1px solid #ccc;
margin-bottom: 1rem;
width: 100%;
}
</style>
Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM

Copy link
Contributor

@kwalrath kwalrath left a comment

Choose a reason for hiding this comment

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

Looks great to me!

@johnpryan
Copy link
Contributor

LGTM

@johnpryan
Copy link
Contributor

johnpryan commented Nov 14, 2019 via email

@chalin
Copy link
Contributor Author

chalin commented Nov 14, 2019

Thanks for the reviews. Travis is green, merging.

@chalin chalin merged commit c8048eb into master Nov 14, 2019
@chalin chalin deleted the chalin-server-get-started-dartpad-1114 branch November 14, 2019 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Contributor has signed the Contributor License Agreement fix.examples Adds or changes example test.general Relates to unit, integration, perf testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants