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

Adds custom font size to list of sizes options #27785

Merged
merged 1 commit into from
Dec 26, 2018

Conversation

cqliu1
Copy link
Contributor

@cqliu1 cqliu1 commented Dec 26, 2018

Closes #24936.

If you set the font size to a value not in our built in list of sizes, the value would display as 0 in the sidebar. This adds a check to add a value to the array of font sizes if the value doesn't already exist.

@cqliu1 cqliu1 added review v7.0.0 Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v6.6.0 v6.5.5 labels Dec 26, 2018
@cqliu1 cqliu1 requested a review from a team as a code owner December 26, 2018 18:27
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-canvas

@elasticmachine
Copy link
Contributor

💔 Build Failed

@w33ble
Copy link
Contributor

w33ble commented Dec 26, 2018

CI failure seems pretty unrelated... was this created from the latest master? Maybe a rebase will cure it? I was having problems last week that seems to have been resolved.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@w33ble
Copy link
Contributor

w33ble commented Dec 26, 2018

Sweet, flakey tests!

Copy link
Contributor

@ryankeairns ryankeairns left a comment

Choose a reason for hiding this comment

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

LGTM. Changed font size and saw it reflected in the settings panel, as described.

@@ -64,6 +64,11 @@ export const TextStylePicker = ({
['underline']: Boolean(underline),
};

if (!isNaN(size) && fontSizes.indexOf(Number(size)) === -1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the isNaN check required here? Invalid values come in as NaNpx (another bug honestly), and then the dropdown still just shows 0. This isn't really a problem (it's edge case and is caused by another problem) and I'm not sure what else it would show, but thought I'd ask.

Copy link
Contributor

Choose a reason for hiding this comment

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

Opened an issue for the NaN thing I mentioned: #27788

Copy link
Contributor

@w33ble w33ble left a comment

Choose a reason for hiding this comment

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

LGTM. Nice fix, I wonder if we have this problem for other drop downs too.

@cqliu1 cqliu1 merged commit 1fd4de0 into elastic:master Dec 26, 2018
cqliu1 added a commit to cqliu1/kibana that referenced this pull request Dec 26, 2018
cqliu1 added a commit to cqliu1/kibana that referenced this pull request Dec 26, 2018
cqliu1 added a commit to cqliu1/kibana that referenced this pull request Dec 26, 2018
clintandrewhall pushed a commit to clintandrewhall/kibana that referenced this pull request Jan 2, 2019
@cqliu1 cqliu1 deleted the fix/font-size-input branch May 6, 2020 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v6.5.5 v6.6.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants