-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Add tour to ccdb #409
Add tour to ccdb #409
Conversation
@anselmbradford this is almost ready. I still have some things I need to work out in the Tour component. Could you take over and assist with the styling? |
src/actions/trends.jsx
Outdated
* @param {string} value the new payload from the tooltip | ||
* @returns {string} a packaged payload to be used by Redux reducers | ||
*/ | ||
export function tourToggled( value ) { |
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 would write a test for this, but it's unnecessary esp if we are going to convert to redux toolkit.
src/components/Tour/Tour.jsx
Outdated
|
||
// hacks to setup the page. we should figure out if we should | ||
// have this in redux instead | ||
if ( expand === 9 ) { |
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 the 9
go into a constant?
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 updated this so it's more scalable so we don't have to hardcode the number.
src/components/Tour/Tour.jsx
Outdated
document.querySelector( '.advanced-container button' ).click() | ||
} | ||
|
||
if ( expand === 16 ) { |
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 the 16
go into a constant?
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.
same as above.
@@ -0,0 +1,16 @@ | |||
// Intro.js styles | |||
@import (css) '../../../node_modules/intro.js/introjs.css'; |
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.
Does @import (css) 'node_modules/intro.js/introjs.css';
work, by chance?
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.
src/components/Tour/TourButton.less
Outdated
@@ -0,0 +1,11 @@ | |||
@import (less) "../../css/base.less"; |
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.
This looks like it could be @import (reference) "../../css/base.less";
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.
this one looks okay.
Looks like the cookie test all of a sudden failing is a known issue |
…-tour # Conflicts: # src/__tests__/utils.spec.jsx
updates responding to GHE 2104 have been made. |
removed Date Range steps in trends and map as requested. |
src/components/RefineBar/Select.jsx
Outdated
@@ -48,7 +48,7 @@ export class Select extends React.Component { | |||
const values = this.getValues() | |||
|
|||
return ( | |||
<section className="cf-select"> | |||
<section className={'cf-select ' + this.props.id}> |
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 this.props.id
go in an id
attribute instead of class?
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.
Ah I see in the snapshot it gets repeated. Well, instead of super generic class names like size
and sort
, let's prefix this to be <section className={'cf-select cf-select__' + this.props.id}>
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 had to add this here so that I could target these for highlighting in intro.js here
I think it might be possible to update the document.querySelector call to look for attributes instead
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.
Hm yea, that actually would be the best way. The prefixed class sets this up as a modifier on cf-select
, which implies that cf-select__sort
(or whatever) is modifying cf-select
. In actuality it is being used as an ID. That's not a big deal, but it could send someone off to look for the modifying CSS. If you want, you should be able to do <section className="cf-select" data-tour={this.props.id}>
and query it with document.querySelector('[data-tour="sort"]')
, or document.querySelector('section[data-tour="sort"]')
.
package.json
Outdated
@@ -149,5 +149,9 @@ | |||
"publishConfig": { | |||
"access": "public", | |||
"registry": "https://registry.npmjs.org/" | |||
}, | |||
"dependencies": { |
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 think we want everything on devDependencies
, as I think dependencies
end up in the npm package. Since the npm package is just delivering a built JS file, I don't think we want anything else there.
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.
You could update to use data- attributes, but otherwise looks good
@@ -44,19 +44,21 @@ export class Select extends React.Component { | |||
} | |||
|
|||
render() { | |||
const id = 'choose-' + this.props.id | |||
const id = 'select-' + this.props.id |
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 just renamed this to select vs choose, since that makes more sense.
I updated it here and also renamed the id to make it clearer. |
Short description explaining the high-level reason for the pull request
Additions
Removals
Changes
Testing
Screenshots
Notes
Todos
Checklist