You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/07/03 00:32:20 UTC

[GitHub] [incubator-superset] rusackas opened a new pull request #10237: chore: Revised logo (new color), w/CSS for vert alignment

rusackas opened a new pull request #10237:
URL: https://github.com/apache/incubator-superset/pull/10237


   ### SUMMARY
   <!--- Describe the change below, including rationale and design decisions -->
   We wanted to update the color of the superset logo. I'm not sure why the existing one had a white stroke, so I used a rasterization of the SVG logo from `superset-frontent/branding`. 
   
   Due to the slightly different size/aspect ratio (which... I actually like), the vertical alignment was a hair off. I added some styles (using Emotion, rather than adding to the LESS codebase) to vertically center logos of any size.
   
   Note that the styles (and thus these screenshots) are for the React menu. I don't _think_ the FAB-generated nav bar is viewable any more, unless I'm mistaken. @nytai could you confirm?
   
   Areas of feature-creep I'm ignoring for this PR, but might circle back to:
   1) Menu.jsx -> Menu.tsx
   2) Removing any styles/jinja templates relating to the old menu that are not visible in superset anymore 
   3) Now that Emotion has entered the picture, there are probably various LESS styles that can be migrated in.
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   <!--- Skip this if not applicable -->
   
   Before:
   ![image](https://user-images.githubusercontent.com/812905/86419511-3ccf1580-bc88-11ea-9c12-1b17fce0a3c0.png)
   
   After:
   ![image](https://user-images.githubusercontent.com/812905/86419487-27f28200-bc88-11ea-83f0-628df920f0ed.png)
   
   
   
   ### TEST PLAN
   <!--- What steps should be taken to verify the changes -->
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] Has associated issue:
   - [ ] Changes UI
   - [ ] Requires DB Migration.
   - [ ] Confirm DB Migration upgrade and downgrade tested.
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   


----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] ktmud commented on a change in pull request #10237: chore: Revised logo (new color), w/CSS for vert alignment

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10237:
URL: https://github.com/apache/incubator-superset/pull/10237#discussion_r450538798



##########
File path: superset-frontend/src/components/Menu/Menu.jsx
##########
@@ -51,11 +52,19 @@ const propTypes = {
   }).isRequired,
 };
 
+const Styles = styled.header`

Review comment:
       I think it's fine to use `Styles`. This is for internal use, kind of like the `<style>` tag in Vue's [Single File Component](https://vuejs.org/v2/guide/single-file-components.html).




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] rusackas commented on pull request #10237: chore: Revised logo (new color), w/CSS for vert alignment

Posted by GitBox <gi...@apache.org>.
rusackas commented on pull request #10237:
URL: https://github.com/apache/incubator-superset/pull/10237#issuecomment-654443048


   > The new logo as a "TM" on it. Is that necessary? I don't see a lot of other websites add a TM to their logo when it's used in the navbar
   
   Removed.
   
   ![image](https://user-images.githubusercontent.com/812905/86637534-8ec6b280-bf8a-11ea-9829-2d81c9321043.png)
   


----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] mistercrunch commented on pull request #10237: chore: Revised logo (new color), w/CSS for vert alignment

Posted by GitBox <gi...@apache.org>.
mistercrunch commented on pull request #10237:
URL: https://github.com/apache/incubator-superset/pull/10237#issuecomment-653574671


   We still use the FAB navbar in some places but it looks ok!
   <img width="725" alt="Screen Shot 2020-07-03 at 7 28 05 AM" src="https://user-images.githubusercontent.com/487433/86478275-de914980-bcfe-11ea-9c4d-40e37f1b19b8.png">
   <img width="1180" alt="Screen Shot 2020-07-03 at 7 27 56 AM" src="https://user-images.githubusercontent.com/487433/86478277-dfc27680-bcfe-11ea-9f3e-5035b0e60901.png">
   
   About `black` failing, you should install pre-commit! https://github.com/apache/incubator-superset/blob/master/CONTRIBUTING.md#git-hooks
   
   I'll stack a commit to try and get CI to pass


----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] rusackas merged pull request #10237: chore: Revised logo (new color), w/CSS for vert alignment

Posted by GitBox <gi...@apache.org>.
rusackas merged pull request #10237:
URL: https://github.com/apache/incubator-superset/pull/10237


   


----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] rusackas commented on a change in pull request #10237: chore: Revised logo (new color), w/CSS for vert alignment

Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10237:
URL: https://github.com/apache/incubator-superset/pull/10237#discussion_r450446451



##########
File path: superset-frontend/images/superset-logo-horiz.svg
##########
@@ -0,0 +1,46 @@
+<?xml version="1.0" encoding="UTF-8" standalone="no"?>
+<!--
+  Licensed to the Apache Software Foundation (ASF) under one
+  or more contributor license agreements.  See the NOTICE file
+  distributed with this work for additional information
+  regarding copyright ownership.  The ASF licenses this file
+  to you 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.
+-->
+<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
+<svg width="100%" height="100%" viewBox="0 0 266 69" version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" xml:space="preserve" xmlns:serif="http://www.serif.com/" style="fill-rule:evenodd;clip-rule:evenodd;stroke-linejoin:round;stroke-miterlimit:2;">
+    <path d="M73.79,15.23C67.32,15.23 61.36,18.87 55.6,25.23C49.94,18.77 43.88,15.23 37.11,15.23C25.9,15.23 17.72,23.23 17.72,34C17.72,44.77 25.9,52.67 37.11,52.67C44,52.67 49.34,49.44 55.3,43C61.06,49.46 66.92,52.69 73.79,52.69C85,52.67 93.18,44.8 93.18,34C93.18,23.2 85,15.23 73.79,15.23ZM37.19,41.37C32.44,41.37 29.61,38.24 29.61,34.1C29.61,29.96 32.44,26.74 37.19,26.74C41.19,26.74 44.46,29.96 48,34.3C44.66,38.34 41.13,41.37 37.19,41.37ZM73.45,41.37C69.51,41.37 66.18,38.24 62.64,34.1C66.28,29.76 69.41,26.74 73.45,26.74C78.2,26.74 81,30 81,34.1C81,38.2 78.2,41.37 73.45,41.37Z" style="fill:rgb(72,72,72);fill-rule:nonzero;"/>
+    <path d="M63.74,50L71.28,41C68.28,40.1 65.51,37.4 62.64,34.05L55.3,43C57.703,45.788 60.556,48.154 63.74,50Z" style="fill:rgb(32,167,201);fill-rule:nonzero;"/>
+    <g id="Main">
+        <g id="Superset">
+            <g id="Full-Lockup-With-Text">
+                <g id="Group-7">
+                    <g id="Group-17">
+                        <g id="Superset-Copy">
+                            <g>

Review comment:
       Meh... I'll just delete it. I'll keep the SVG trimming in mind if/when it makes a comeback.




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] etr2460 commented on a change in pull request #10237: chore: Revised logo (new color), w/CSS for vert alignment

Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #10237:
URL: https://github.com/apache/incubator-superset/pull/10237#discussion_r449665356



##########
File path: superset-frontend/images/superset-logo-horiz.svg
##########
@@ -0,0 +1,46 @@
+<?xml version="1.0" encoding="UTF-8" standalone="no"?>

Review comment:
       Where is the SVG used? I don't see it being used anywhere

##########
File path: superset-frontend/images/superset-logo-horiz.svg
##########
@@ -0,0 +1,46 @@
+<?xml version="1.0" encoding="UTF-8" standalone="no"?>
+<!--
+  Licensed to the Apache Software Foundation (ASF) under one
+  or more contributor license agreements.  See the NOTICE file
+  distributed with this work for additional information
+  regarding copyright ownership.  The ASF licenses this file
+  to you 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.
+-->
+<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
+<svg width="100%" height="100%" viewBox="0 0 266 69" version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" xml:space="preserve" xmlns:serif="http://www.serif.com/" style="fill-rule:evenodd;clip-rule:evenodd;stroke-linejoin:round;stroke-miterlimit:2;">
+    <path d="M73.79,15.23C67.32,15.23 61.36,18.87 55.6,25.23C49.94,18.77 43.88,15.23 37.11,15.23C25.9,15.23 17.72,23.23 17.72,34C17.72,44.77 25.9,52.67 37.11,52.67C44,52.67 49.34,49.44 55.3,43C61.06,49.46 66.92,52.69 73.79,52.69C85,52.67 93.18,44.8 93.18,34C93.18,23.2 85,15.23 73.79,15.23ZM37.19,41.37C32.44,41.37 29.61,38.24 29.61,34.1C29.61,29.96 32.44,26.74 37.19,26.74C41.19,26.74 44.46,29.96 48,34.3C44.66,38.34 41.13,41.37 37.19,41.37ZM73.45,41.37C69.51,41.37 66.18,38.24 62.64,34.1C66.28,29.76 69.41,26.74 73.45,26.74C78.2,26.74 81,30 81,34.1C81,38.2 78.2,41.37 73.45,41.37Z" style="fill:rgb(72,72,72);fill-rule:nonzero;"/>
+    <path d="M63.74,50L71.28,41C68.28,40.1 65.51,37.4 62.64,34.05L55.3,43C57.703,45.788 60.556,48.154 63.74,50Z" style="fill:rgb(32,167,201);fill-rule:nonzero;"/>
+    <g id="Main">
+        <g id="Superset">
+            <g id="Full-Lockup-With-Text">
+                <g id="Group-7">
+                    <g id="Group-17">
+                        <g id="Superset-Copy">
+                            <g>

Review comment:
       let's get rid of some of these unnecessary `g` tags?

##########
File path: superset-frontend/src/components/Menu/Menu.jsx
##########
@@ -51,11 +52,19 @@ const propTypes = {
   }).isRequired,
 };
 
+const Styles = styled.header`

Review comment:
       Maybe a name like `StyledHeader` might be more clear than `Styles`?




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] rusackas commented on a change in pull request #10237: chore: Revised logo (new color), w/CSS for vert alignment

Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10237:
URL: https://github.com/apache/incubator-superset/pull/10237#discussion_r450443969



##########
File path: superset-frontend/images/superset-logo-horiz.svg
##########
@@ -0,0 +1,46 @@
+<?xml version="1.0" encoding="UTF-8" standalone="no"?>

Review comment:
       It's basically vestigial. The "thing I wanted to try" was editing this SVG file to use "currentColor" for the blue fill areas, such that the SVG inherits the brandColor variable. Then if people wanted to restyle their superset instance to use orange (or whatever) in the brand color, it'd be reflected in the logo.
   
   However, that basically requires building in some JSX that depends on the logo using SVG. I'm sure lots of people are using JPG/PNG logos, so a big revamp around this suddenly out of scope.
   
   TL;DR: it's a bit vestigial for now, but I hope to circle back and make good use of it. I can remove it from the PR if it's a bother.




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] codecov-commenter commented on pull request #10237: chore: Revised logo (new color), w/CSS for vert alignment

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #10237:
URL: https://github.com/apache/incubator-superset/pull/10237#issuecomment-653588478


   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10237?src=pr&el=h1) Report
   > Merging [#10237](https://codecov.io/gh/apache/incubator-superset/pull/10237?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/4281ad5486f976107b3fb3637519defe63133b9b&el=desc) will **decrease** coverage by `6.25%`.
   > The diff coverage is `100.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/10237/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/incubator-superset/pull/10237?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #10237      +/-   ##
   ==========================================
   - Coverage   65.70%   59.44%   -6.26%     
   ==========================================
     Files         594      404     -190     
     Lines       31501    13108   -18393     
     Branches     3221     3221              
   ==========================================
   - Hits        20697     7792   -12905     
   + Misses      10623     5135    -5488     
     Partials      181      181              
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | #javascript | `59.44% <100.00%> (-0.01%)` | :arrow_down: |
   | #python | `?` | |
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/10237?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [superset-frontend/src/components/Menu/Menu.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10237/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTWVudS9NZW51LmpzeA==) | `88.23% <100.00%> (+0.73%)` | :arrow_up: |
   | [...set-frontend/src/views/datasetList/DatasetList.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10237/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2RhdGFzZXRMaXN0L0RhdGFzZXRMaXN0LnRzeA==) | `68.25% <0.00%> (-0.80%)` | :arrow_down: |
   | [superset/sql\_validators/\_\_init\_\_.py](https://codecov.io/gh/apache/incubator-superset/pull/10237/diff?src=pr&el=tree#diff-c3VwZXJzZXQvc3FsX3ZhbGlkYXRvcnMvX19pbml0X18ucHk=) | | |
   | [superset/db\_engine\_specs/hana.py](https://codecov.io/gh/apache/incubator-superset/pull/10237/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hhbmEucHk=) | | |
   | [superset/views/chart/mixin.py](https://codecov.io/gh/apache/incubator-superset/pull/10237/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvY2hhcnQvbWl4aW4ucHk=) | | |
   | [superset/cli.py](https://codecov.io/gh/apache/incubator-superset/pull/10237/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY2xpLnB5) | | |
   | [superset/dao/base.py](https://codecov.io/gh/apache/incubator-superset/pull/10237/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGFvL2Jhc2UucHk=) | | |
   | [superset/views/chart/views.py](https://codecov.io/gh/apache/incubator-superset/pull/10237/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvY2hhcnQvdmlld3MucHk=) | | |
   | [superset/dashboards/dao.py](https://codecov.io/gh/apache/incubator-superset/pull/10237/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9kYW8ucHk=) | | |
   | [superset/charts/api.py](https://codecov.io/gh/apache/incubator-superset/pull/10237/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY2hhcnRzL2FwaS5weQ==) | | |
   | ... and [181 more](https://codecov.io/gh/apache/incubator-superset/pull/10237/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10237?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10237?src=pr&el=footer). Last update [4281ad5...8f4c0ec](https://codecov.io/gh/apache/incubator-superset/pull/10237?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] nytai commented on pull request #10237: chore: Revised logo (new color), w/CSS for vert alignment

Posted by GitBox <gi...@apache.org>.
nytai commented on pull request #10237:
URL: https://github.com/apache/incubator-superset/pull/10237#issuecomment-653275125


   @rusackas I believe the FAB generated menu is still used on certain route, eg the FAB generated ones for example (try `/databaseview/list/` or `/csvtodatabaseview/form`). We really should migrate off the old FAB menu, it's getting pretty confusing. I'll take a look and see what's possible in the coming days. 


----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org