TheNeuralBit commented on a change in pull request #13565:
URL: https://github.com/apache/beam/pull/13565#discussion_r568261897
##########
File path: website/www/site/content/en/blog/adding-data-sources-to-sql.md
##########
@@ -81,19 +81,19 @@ The `TableProvider` classes are under
Our table provider looks like this:
-{{< highlight java >}}
+{{< highlight language="java" >}}
@AutoService(TableProvider.class)
public class GenerateSequenceTableProvider extends InMemoryMetaTableProvider {
- @Override
- public String getTableType() {
- return "sequence";
- }
+@Override
+public String getTableType() {
+return "sequence";
+}
- @Override
- public BeamSqlTable buildBeamSqlTable(Table table) {
- return new GenerateSequenceTable(table);
- }
+@Override
+public BeamSqlTable buildBeamSqlTable(Table table) {
+return new GenerateSequenceTable(table);
+}
Review comment:
I don't think we should be making changes to a blog post's contents. It
looks like you've made changes to a lot of blog posts to remove leading
whitespace in code snippets.
Are these changes something you did while you were experimenting? Can they
be reverted?
##########
File path: website/www/site/content/en/blog/adding-data-sources-to-sql.md
##########
@@ -81,19 +81,19 @@ The `TableProvider` classes are under
Our table provider looks like this:
-{{< highlight java >}}
+{{< highlight language="java" >}}
@AutoService(TableProvider.class)
public class GenerateSequenceTableProvider extends InMemoryMetaTableProvider {
- @Override
- public String getTableType() {
- return "sequence";
- }
+@Override
+public String getTableType() {
+return "sequence";
+}
- @Override
- public BeamSqlTable buildBeamSqlTable(Table table) {
- return new GenerateSequenceTable(table);
- }
+@Override
+public BeamSqlTable buildBeamSqlTable(Table table) {
+return new GenerateSequenceTable(table);
+}
Review comment:
Can we just set up the website to use the new code snippet widget with
```? Unless we add some explicit check to forbid ```, people will continue to
use that syntax since it's very common markdown syntax.
##########
File path: website/www/site/content/en/blog/adding-data-sources-to-sql.md
##########
@@ -81,19 +81,19 @@ The `TableProvider` classes are under
Our table provider looks like this:
-{{< highlight java >}}
+{{< highlight language="java" >}}
@AutoService(TableProvider.class)
public class GenerateSequenceTableProvider extends InMemoryMetaTableProvider {
- @Override
- public String getTableType() {
- return "sequence";
- }
+@Override
+public String getTableType() {
+return "sequence";
+}
- @Override
- public BeamSqlTable buildBeamSqlTable(Table table) {
- return new GenerateSequenceTable(table);
- }
+@Override
+public BeamSqlTable buildBeamSqlTable(Table table) {
+return new GenerateSequenceTable(table);
+}
Review comment:
Can we just set up the website to use the new code snippet widget with
\```? Unless we add some explicit check to forbid ```, people will continue to
use that syntax since it's very common markdown syntax.
##########
File path: website/www/site/data/en/ways_of_contribution.yaml
##########
@@ -0,0 +1,21 @@
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+- title: Beam Technical committers
+ body: Short description with hyperlinks.
+ icon: icons/contributor/become a committer/code-icon.svg
+- title: Non-technical committers
+ body: Short description with hyperlinks.
+ icon: icons/contributor/become a committer/file-icon.svg
+- title: Website maintainers
+ body: Short description with hyperlinks.
+ icon: icons/contributor/become a committer/tool-icon.svg
Review comment:
Still need some copy here
##########
File path: website/www/site/layouts/shortcodes/highlight.html
##########
@@ -12,20 +12,36 @@
{{ $content := (trim .Inner "\n\r") | htmlUnescape | safeHTML }}
{{ $ctx := . }}
-{{ with (.Get "class") }}
- <div class={{ . }}>
- {{ with ($ctx.Get "lang") }}
+{{ $py := .Get "py" }}
+{{ with (.Get "language") }}
+ <div class='{{ printf "language-%s" . }} snippet'>
+ <div class="notebook-skip code-snippet">
+ {{ with $py }}
+ <a target="_blank" type="button" data-bs-toggle="tooltip"
data-bs-placement="bottom" title="Run code"
href="https://colab.research.google.com/github/{{ $ctx.Site.Params.branch_repo
}}/{{ $ctx.Get `notebook` }}-py.ipynb">
+ <img src="/images/run-icon.svg"/>
+ </a>
+ <a target="_blank" type="button" data-bs-toggle="tooltip"
data-bs-placement="bottom" title="View source code" href="https://github.com/{{
$ctx.Site.Params.branch_repo }}/{{ $ctx.Get `py` }}">
+ <img src="/images/code-icon.svg"/>
+ </a>
+ <a class="copy" type="button" data-bs-toggle="tooltip"
data-bs-placement="bottom" title="Copy to clipboard">
+ <img src="/images/copy-icon.svg"/>
+ </a>
+ {{ else }}
+ <a class="copy" type="button" data-bs-toggle="tooltip"
data-bs-placement="bottom" title="Copy to clipboard">
+ <img src="/images/copy-icon.svg"/>
+ </a>
+ {{ end }}
{{ highlight $content . "" }}
- {{ else }}
- {{ highlight $content "" "" }}
- {{ end }}
+ </div>
</div>
{{ else }}
- {{ with (.Get 0) }}
- <div class={{ printf "language-%s" . }}>
- {{ highlight $content . "" }}
+ <div class="snippet">
+ <div class="notebook-skip code-snippet without_switcher">
+ <a class="copy" type="button" data-bs-toggle="tooltip"
data-bs-placement="bottom" title="Copy to clipboard">
+ <img src="/images/copy-icon.svg"/>
+ </a>
+ {{ highlight $content "" "" }}
</div>
- {{ else }}
- {{ highlight $content "" "" }}
- {{ end }}
+ </div>
Review comment:
There are a few odd things about the way this code snippet template
renders for me:
- There's an extra horizaontal line at the top of the snippet that extends
off to the right
- It looks like there's a 1px gap between the tabs and the top of the
snippet, which looks weird to me
- There's a lot of empty space before the first line of code (and to a
lesser extent, after the last line of code), I think this looks pretty clunky,
especially for snippets that have just one line.
##########
File path: website/www/site/layouts/shortcodes/highlight.html
##########
@@ -12,20 +12,36 @@
{{ $content := (trim .Inner "\n\r") | htmlUnescape | safeHTML }}
{{ $ctx := . }}
-{{ with (.Get "class") }}
- <div class={{ . }}>
- {{ with ($ctx.Get "lang") }}
+{{ $py := .Get "py" }}
+{{ with (.Get "language") }}
+ <div class='{{ printf "language-%s" . }} snippet'>
+ <div class="notebook-skip code-snippet">
+ {{ with $py }}
+ <a target="_blank" type="button" data-bs-toggle="tooltip"
data-bs-placement="bottom" title="Run code"
href="https://colab.research.google.com/github/{{ $ctx.Site.Params.branch_repo
}}/{{ $ctx.Get `notebook` }}-py.ipynb">
+ <img src="/images/run-icon.svg"/>
+ </a>
+ <a target="_blank" type="button" data-bs-toggle="tooltip"
data-bs-placement="bottom" title="View source code" href="https://github.com/{{
$ctx.Site.Params.branch_repo }}/{{ $ctx.Get `py` }}">
+ <img src="/images/code-icon.svg"/>
+ </a>
+ <a class="copy" type="button" data-bs-toggle="tooltip"
data-bs-placement="bottom" title="Copy to clipboard">
+ <img src="/images/copy-icon.svg"/>
+ </a>
+ {{ else }}
+ <a class="copy" type="button" data-bs-toggle="tooltip"
data-bs-placement="bottom" title="Copy to clipboard">
+ <img src="/images/copy-icon.svg"/>
+ </a>
+ {{ end }}
{{ highlight $content . "" }}
- {{ else }}
- {{ highlight $content "" "" }}
- {{ end }}
+ </div>
</div>
{{ else }}
- {{ with (.Get 0) }}
- <div class={{ printf "language-%s" . }}>
- {{ highlight $content . "" }}
+ <div class="snippet">
+ <div class="notebook-skip code-snippet without_switcher">
+ <a class="copy" type="button" data-bs-toggle="tooltip"
data-bs-placement="bottom" title="Copy to clipboard">
+ <img src="/images/copy-icon.svg"/>
+ </a>
+ {{ highlight $content "" "" }}
</div>
- {{ else }}
- {{ highlight $content "" "" }}
- {{ end }}
+ </div>
Review comment:
I'd say the only one of these that's a blocker is the horizontal line
extending off to the right.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
