-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
chore: Remove logo forced width #19049
Conversation
Codecov Report
@@ Coverage Diff @@
## master #19049 +/- ##
==========================================
- Coverage 66.57% 66.57% -0.01%
==========================================
Files 1667 1666 -1
Lines 64421 64328 -93
Branches 6503 6492 -11
==========================================
- Hits 42886 42824 -62
+ Misses 19850 19822 -28
+ Partials 1685 1682 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
@@ -236,7 +236,6 @@ def _try_json_readsha(filepath: str, length: int) -> Optional[str]: | |||
|
|||
# Specify the App icon | |||
APP_ICON = "/static/assets/images/superset-logo-horiz.png" | |||
APP_ICON_WIDTH = 126 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add something to UPDATING.MD
stating that this configuration was removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right! Added to the breaking changes. Thanks @michael-s-molina
@rusackas let me know your thoughts about communicating this breaking change to the wider community before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do this, I think we should tag the PR with the 2.0
label and the risk:breaking-change
label, add it to the 2.0 project board, and update UPDATING.MD as you've done here.
Or, as noted elsewhere, we COULD use that value in the CSS, to make it a little more configurable. If we want to keep that complexity and avoid the breaking change, that is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rusackas I think the configurability brings a bit of complexity to it. If there is a strong signal from the community that they need the APP_ICON_WIDTH
, we should probably add another config flag, such as USE_APP_ICON_FORCED_WIDTH = Boolean
, and then behave consistently with that when True. When False just fallback to the implementation of this PR.
@rusackas and I were playing with the
Before the changes: Screen.Recording.2022-03-08.at.4.12.13.PM.movAfter the changes: Screen.Recording.2022-03-08.at.4.21.01.PM.movWhat do you think? @rusackas I know that you mentioned calculating the left and right paddings from the icon's width but do you think is necessary or this is enough? Maybe it is worth a shot. |
/testenv up |
@rusackas Ephemeral environment spinning up at http://34.220.105.158:8080. Credentials are |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving, as this seems like a sensible change. The width removal is super nitpicky thing to be considered a breaking change, but it's POSSIBLE someone is using the app config to do something specific and this will change their layout by a pixel here or a pixel there.
This was not voted in as part of the 2.0 SIP, but I think it's minor enough that we might consider it a late addition, and just label the PR appropriately with 2.0
and risk:breaking0change
. Curious the thoughts of @john-bodley and @etr2460 or any other folks from the community on the relative risk or addition of a change while we're in the 2.0 build window.
`${theme.gridUnit}px ${theme.gridUnit * 2}px ${theme.gridUnit}px ${ | ||
theme.gridUnit * 4 | ||
}px`}; | ||
max-width: ${({ theme }) => `${theme.gridUnit * 37}px`}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm on the fence about this... 37 seems like a pretty arbitrary number. We could use the APP_ICON_WIDTH
here, I suppose, as much as I'd like to remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with using the APP_ICON_WIDTH
here is that it would not behave the same as it used to. I think best is not to use it or change it with APP_ICON_MAX_WIDTH
for clarity, if that can be of any benefit to the users
@@ -97,6 +96,17 @@ const StyledHeader = styled.header` | |||
display: flex; | |||
flex-direction: column; | |||
justify-content: center; | |||
min-height: ${({ theme }) => `${theme.gridUnit * 12.5}px`}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just 12? Or lucky 13? Trying to avoid fractional gridUnits wherever possible, as usual.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope. It needs to be exactly 50 or it will break the antdesign nav bar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we just forgo the gridUnit here and give it a pixel measurement (and maybe a comment for the rationale). Then it won't be fragile if someone changes the gridUnit from 4px to 5px in their theme.
@@ -236,7 +236,6 @@ def _try_json_readsha(filepath: str, length: int) -> Optional[str]: | |||
|
|||
# Specify the App icon | |||
APP_ICON = "/static/assets/images/superset-logo-horiz.png" | |||
APP_ICON_WIDTH = 126 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do this, I think we should tag the PR with the 2.0
label and the risk:breaking-change
label, add it to the 2.0 project board, and update UPDATING.MD as you've done here.
Or, as noted elsewhere, we COULD use that value in the CSS, to make it a little more configurable. If we want to keep that complexity and avoid the breaking change, that is.
I agree that removing a config option should be a breaking change, and thus this should be added in UPDATING.md and tagged for superset 2.0 |
Fixes #19105 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates
@geido this just needs a rebase to pass CI, I believe. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another approval 😁
I keep trying to merge this thing, but UPDATING.md keeps causing conflicts. The most frustrating kind of progress :) Here we go again... go, CI, go!! |
Updating became a merge conflict AGAIN. Go, CI, go! |
Ephemeral environment shutdown and build artifacts deleted. |
* Remove logo forced width * improve styling * improve paddings * Update UPDATING.md * Fixes a typo * Add fixed height Co-authored-by: Michael S. Molina <[email protected]> Co-authored-by: Evan Rusackas <[email protected]>
SUMMARY
This PR removes the necessity of adding a forced width to the
APP_ICON
image.Fixes #19105
AFTER
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION