Show the shorter version of the empty annotations/notes message when … #611
Conversation
Codecov Report
@@ Coverage Diff @@
## master #611 +/- ##
==========================================
+ Coverage 90.91% 90.94% +0.02%
==========================================
Files 133 134 +1
Lines 5351 5357 +6
Branches 931 930 -1
==========================================
+ Hits 4865 4872 +7
+ Misses 486 485 -1
Continue to review full report at Codecov.
|
|
@robertknight I think |
| /** | ||
| * Returns true if the sidebar tutorial has to be shown to a user for a given session. | ||
| */ | ||
| function show(sessionState) { |
There was a problem hiding this comment.
-
The
preferencesproperty is a non-optional field in the profile type as returned by/api/profile, so you don't need to check for it. -
Re. your question about where this should go, the
src/sidebar/filterdirectory refers to Angular filters, rather than utility functions. There are two places in the code for this kind of logic:- A simple function in a module in the
util/directory (eg. in sayutil/sessionor something). If you do this, please make sure the function name makes sense on its own (eg.shouldShowSidebarTutorial). - A "selector" function in an app state module (eg.
src/sidebar/reducers/sessionin this case) which takes the app state and returns something derived from it.
- A simple function in a module in the
Also, one related note - the name "session" in various modules is a hangover from when the data came from h's /app endpoint, which returned data about the user's cookie session on the website. At some point we should change it to "profile" everywhere since it comes from /api/profile now and no longer necessarily reflects the user's state on the website (eg. if using third party accounts).
| totalAnnotations: '10', | ||
| totalNotes: 0, | ||
| }); | ||
| var unavailableMsg = elem[0].querySelectorAll('.annotation-unavailable-message__label')[0]; |
There was a problem hiding this comment.
You can use querySelector(selector) rather than querySelectorAll(selector)[0] if you are only interested in the first element.
| assert.isTrue(noteIcon.classList.contains('h-icon-annotate')); | ||
| }); | ||
|
|
||
| it('should not display the new note button when the notes tab is active and the new note button is disabled', function () { |
There was a problem hiding this comment.
These tests are in a when the sidebar tutorial is not displayed context block, but they apply whether or not the sidebar tutorial is shown, don't they? If so, then they should be outside the context block. Also, don't we already have tests for this?
There was a problem hiding this comment.
This works as expected. I did encounter some browser-specific (Safari #599, Firefox) issues with the rendering of the "New note" button but these already exist in master.
I left a few comments about the code to address.
…the sidebar tutorial is displayed and the new note button is enabled. Fixes #610
fe2086c to
7a86766
Compare
|
|
||
| assert.include(msg.textContent, 'There are no page notes in this group.'); | ||
| assert.notInclude(msg.textContent, 'Create one by clicking the'); | ||
| assert.notInclude(msg.textContent, 'Create one by selecting some text and clicking the'); |
There was a problem hiding this comment.
I think this line was included by mistake? You never need to select text to create a note.
| var msg = elem[0].querySelector('.annotation-unavailable-message__label'); | ||
|
|
||
| assert.include(msg.textContent, 'There are no annotations in this group.'); | ||
| assert.notInclude(msg.textContent, 'Create one by clicking the'); |
There was a problem hiding this comment.
It looks like this line is a mistake as well, since "Create one by clicking the" is only shown in the notes tab.
Just as a suggestion, it might save a few lines and make the tests more readable to extract out the logic for querying the DOM into a helper function which returns some value to indicate the displayed message that you can then test for. Alternatively if the template "logic" is very simple (eg. hiding/showing different elements depending on some controller methods which return a boolean) you can might be able to get away with just testing the controller method's output in different cases.
There was a problem hiding this comment.
Discussed this on Slack. This is a check to make sure that the annotations-related message is not showing.
…the sidebar tutorial is displayed
and the new note button is enabled.
Fixes #610