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.

Sign up for GitHub

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

Jump to bottom

Give "Reason" a reference tooltip #501

Merged
merged 6 commits into from
Mar 11, 2020
Merged

Give "Reason" a reference tooltip #501

merged 6 commits into from
Mar 11, 2020

Conversation

arkadyan
Copy link
Contributor

@arkadyan arkadyan commented Mar 11, 2020 •
edited
Loading

Add a question mark icon.

Screen Shot 2020-03-10 at 14 51 49

Screen Shot 2020-03-11 at 15 02 23

@arkadyan
Copy link
Contributor Author

This is currently on dev for testing (the tooltip will not show up running in your local development server).

@arkadyan arkadyan force-pushed the mss-reason-hint-button branch from ca4f37b to 9912de7 Compare March 11, 2020 15:10
@arkadyan arkadyan force-pushed the mss-reason-hint-button branch from 9912de7 to cd6e975 Compare March 11, 2020 16:12
@codecov
Copy link

codecov bot commented Mar 11, 2020 •
edited
Loading

Codecov Report

Merging #501 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #501   +/-   ##
=======================================
  Coverage   97.70%   97.70%           
=======================================
  Files         169      169           
  Lines        3750     3755    +5     
  Branches      516      517    +1     
=======================================
+ Hits         3664     3669    +5     
  Misses         83       83           
  Partials        3        3
Impacted Files Coverage Δ
...c/components/propertiesPanel/blockWaiverBanner.tsx 100.00% <100.00%> (ø)
assets/src/components/tabBar.tsx 100.00% <100.00%> (ø)
assets/src/helpers/icon.tsx 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb6aae5...edae958. Read the comment docs.

@ingridpierre
Copy link

ingridpierre commented Mar 11, 2020 •
edited
Loading

@arkadyan can we make "Reason" display as "REASON" to match the other little labels below?

eta: font-weight: 500 also

@arkadyan
Copy link
Contributor Author

@ingridpierre Good catch, I fixed the uppercase thing.

@ingridpierre
Copy link

One more lil tweak, current waivers should use green in the label. like so:

Current:
image
Desired:
image

@arkadyan arkadyan force-pushed the mss-reason-hint-button branch from a000b22 to 265700f Compare March 11, 2020 19:07
@arkadyan
Copy link
Contributor Author

@ingridpierre @skyqrose Updated to use a local tooltip instead of Appcues.

@arkadyan arkadyan changed the title Turn "Reason" into an Appcues hint button Give "Reason" a reference tooltip Mar 11, 2020
@ingridpierre
Copy link

"Operator Error" should be "Operator error" for consistency.

skyqrose
skyqrose approved these changes Mar 11, 2020
View reviewed changes
Copy link
Member

@skyqrose skyqrose left a comment

Choose a reason for hiding this comment

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

I like it! It works so smoothly, much better than Appcues.

Approval pending a 👍 from Akira.

assets/css/properties_panel/_block_waiver_banner.scss Outdated

.m-block-waiver-banner__reason-tooltip.__react_component_tooltip.show {
opacity: 1;
text-align: left;
Copy link
Member

Choose a reason for hiding this comment

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

With the ladder styling moved back to be specifically about the ladder, this property isn't needed anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, removed.

assets/src/skate.d.ts Outdated
@@ -5,6 +5,7 @@ declare global {
Appcues?: {
identify: (userId: string) => void
page: () => void
show: (id: string) => void
Copy link
Member

Choose a reason for hiding this comment

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

I don't see this function called anywhere. What happens to the tests if it's not mocked?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, that is no longer needed.

@arkadyan
Copy link
Contributor Author

@ingridpierre thanks, made error lowercase.

@ingridpierre
Copy link

@arkadyan lgtm 👍

@arkadyan arkadyan merged commit 5acf8ad into master Mar 11, 2020
@arkadyan arkadyan deleted the mss-reason-hint-button branch March 11, 2020 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skyqrose skyqrose skyqrose approved these changes

@ingridpierre ingridpierre Awaiting requested review from ingridpierre

Assignees

@ingridpierre ingridpierre

@skyqrose skyqrose

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@arkadyan @ingridpierre @skyqrose