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

Added tour of viewer tools for better User experience #334

Merged
merged 5 commits into from
Apr 7, 2020

Conversation

Hemansh31
Copy link
Contributor

I added a bootstrap tour of all the tools
Screencast
CaMicroscope Data Table (1)

@birm birm requested a review from nanli-emory April 6, 2020 21:14
Copy link
Member

@birm birm left a comment

Choose a reason for hiding this comment

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

add the apps/viewer/bootstrap-tour-standalone.min.js to common, and add it to the eslint ignore file

Copy link
Member

@birm birm left a comment

Choose a reason for hiding this comment

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

How many of the style issues can you fix with npm run-script lint-fix

Copy link
Contributor

@cjchirag7 cjchirag7 left a comment

Choose a reason for hiding this comment

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

  1. To remove ESLint errors, firstly go inside your caMicroscope folder, enter the following commands into terminal.
npm install
npm run lint-fix

I have tested this. This would remove almost 90 lint errors and you will remain with only 13 of them, which can be easily removed,as 12 of them say This line has a length of x. Maximum allowed is 120 and one says Multiline support is limited to browsers supporting ES5 only. You would just need to split them into different lines, or use template literals (enclosing the string in back-ticks ` )

  1. After introducing the changes, I requested in tut.js and styles.css , the tour would look like this -
    new-tour

.eslintignore Outdated
@@ -11,3 +11,4 @@ apps/landing/jquery.dropotron.min.js
apps/landing/jquery.min.js
apps/landing/jquery.scrollex.min.js
apps/segment/opencv.js
common/bootstrap-tour-standalone.min.js
Copy link
Contributor

@cjchirag7 cjchirag7 Apr 7, 2020

Choose a reason for hiding this comment

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

@Hemansh31 , it would be better to make a folder named 'bootstrap-tour-standalone' inside the common folder and place both bootstrap-tour-standalone.min.css and bootstrap-tour-standalone.min.js inside that folder. As, this would be a common component for tours and can be used for developing tours for other components like heatmaps, etc. also

And BTW, if you remove this line, it would be better as ESLINT is configured to test only .js files inside components, core and apps folders, so it is not testing the .js files in components folder already (Check travis.yml file for this)

Suggested change
common/bootstrap-tour-standalone.min.js

Comment on lines 105 to 106
</div><div style='width:100%;display: flex;margin-bottom: 5px;'><button class='btn btn-default' data-role='end' style='margin:auto;'>End tour</button></div></div>"
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
</div><div style='width:100%;display: flex;margin-bottom: 5px;'><button class='btn btn-default' data-role='end' style='margin:auto;'>End tour</button></div></div>"
});
</div><div style='width:100%;display: flex;margin-bottom: 5px;'><button class='btn btn-default' data-role='end' style='margin:auto;'>End tour</button></div></div>",
backdrop: true
},
);

Including backdrop would make it look better but along with this, please also add these lines in the 'styles.css' in css folder -

.tour-step-background {
background: #71daec !important;
opacity: 0.2 !important;
}

@@ -160,7 +160,7 @@
media="all"
href="../../css/popup.css"
/>

<link href="./bootstrap-tour-standalone.min.css" rel="stylesheet">
Copy link
Contributor

Choose a reason for hiding this comment

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

After creating the folder in common, you would need to change this.

Suggested change
<link href="./bootstrap-tour-standalone.min.css" rel="stylesheet">
<link href="../../common/bootstrap-tour-standalone/bootstrap-tour-standalone.min.css" rel="stylesheet">

@@ -324,6 +324,8 @@
<script src="./uicallbacks.js"></script>
<script src="./dataloaders.js"></script>
<script src="./init.js"></script>
<script src="../../common/bootstrap-tour-standalone.min.js"></script>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<script src="../../common/bootstrap-tour-standalone.min.js"></script>
<script src="../../common/bootstrap-tour-standalone/bootstrap-tour-standalone.min.js"></script>

@nanli-emory
Copy link
Member

@Hemansh31 I love this feature. It will help a lot to users. The only thing that you need to change is the opacity of the background. Could you make it more transparent (.3 ~ .5)?

@Hemansh31
Copy link
Contributor Author

@cjchirag7 Thanks for the valuable suggestions

@birm birm merged commit a5e0fac into camicroscope:develop Apr 7, 2020
Hemansh31 added a commit to Hemansh31/caMicroscope that referenced this pull request Apr 7, 2020
Added details of Tutorial icon [PR camicroscope#334]
@Hemansh31 Hemansh31 mentioned this pull request Apr 7, 2020
birm pushed a commit that referenced this pull request Apr 7, 2020
Added Tutorial icon from [PR #334]
@birm birm mentioned this pull request Apr 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants