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

fix(copy-button): fix type of feedback text #4677

Merged
merged 6 commits into from
Nov 19, 2019

Conversation

asudoh
Copy link
Contributor

@asudoh asudoh commented Nov 14, 2019

Fixes #4606.

Changelog

Changed

  • Choice of type token for feedback text.

Testing / Reviewing

Testing should make sure copy button and code snippet is not broken.

@netlify
Copy link

netlify bot commented Nov 14, 2019

Deploy preview for the-carbon-components ready!

Built with commit 7fbd19b

https://deploy-preview-4677--the-carbon-components.netlify.com

@netlify
Copy link

netlify bot commented Nov 14, 2019

Deploy preview for carbon-elements ready!

Built with commit 7fbd19b

https://deploy-preview-4677--carbon-elements.netlify.com

@netlify
Copy link

netlify bot commented Nov 14, 2019

Deploy preview for carbon-components-react ready!

Built with commit 7fbd19b

https://deploy-preview-4677--carbon-components-react.netlify.com

Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

copy button and inline code snippet look correct, but single line and multiline code snippet feedback text still appear to be using a different type token

image

image

@abbeyhrt
Copy link
Contributor

I'm seeing the same thing as @emyarod

Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

Vanilla and React:

-The code snippet single line and the copy button should 4px between the border of the hover and the tip of the fin of the tooltip. Right now its too close.
Screen Shot 2019-11-14 at 1 22 25 PM

-Seeing the same thing as andrew and abbey about the type

Copy link
Contributor

@joshblack joshblack left a comment

Choose a reason for hiding this comment

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

Seems great with feedback from above 😄

Copy link
Contributor

@joshblack joshblack left a comment

Choose a reason for hiding this comment

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

Any chance we could avoid the magic numbers or document them? Seems like we're setting up the placement to be very precise with the decimal pixels

@@ -302,9 +302,23 @@
}

.#{$prefix}--snippet-button .#{$prefix}--btn--copy__feedback {
top: rem(25px);
top: rem(50.8px);
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this number coming from?

.#{$prefix}--snippet--multi
.#{$prefix}--snippet-button
.#{$prefix}--btn--copy__feedback {
top: rem(42.8px);
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this number coming from?

@asudoh asudoh requested a review from a team as a code owner November 19, 2019 03:20
@ghost ghost requested a review from emyarod November 19, 2019 03:20
@asudoh
Copy link
Contributor Author

asudoh commented Nov 19, 2019

Fixed the tooltip offset, and added the comment how the right offset is generated. Made a change to force serif font as our type token of choice doesn't dictate the font face and code snippet UI has monospace font.

Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

the magic numbers in code snippet should be resolved by pending work for #4605 right?

on a side note the copy button tooltip positioning should be resolved in #4715

Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

placement of tooltip and the type looks good to me 👍

…scss

Co-Authored-By: emyarod <emyarod@users.noreply.github.com>
@asudoh asudoh merged commit 980b550 into carbon-design-system:master Nov 19, 2019
@asudoh asudoh deleted the copy-button-feedback-text branch November 19, 2019 22:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(code snippet) multi/single line have incorrect feedback message type token
5 participants