Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
chore: Remove logo forced width #19049
Changes from 4 commits
aa26936
67ec9bf
357df31
f777d61
ebc96cd
99c9c67
04147bf
36c5650
a4acf42
40ea796
687a26d
c588534
62e1d43
8c926f4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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 withAPP_ICON_MAX_WIDTH
for clarity, if that can be of any benefit to the usersThere 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 therisk: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 asUSE_APP_ICON_FORCED_WIDTH = Boolean
, and then behave consistently with that when True. When False just fallback to the implementation of this PR.