Add note's tags to history screen #2817
Conversation
There was a problem hiding this comment.
This is looking good! I really appreciate the updated and added tests for the tag chip.
The one thing I noticed is that it seems we are losing the aria-hidden=true on the note revisions which was recently added for the preview when the revision selector is open.
Oh good point! I'll check the recent improvements on the navigation accessibility from the PR because I'm not very familiar with ARIA, thanks! |
lib/note-revisions/index.tsx
Outdated
| getTagName = (tag: T.TagName) => { | ||
| const { allTags } = this.props; | ||
| return allTags.get(tagHashOf(tag))?.name; | ||
| }; |
There was a problem hiding this comment.
there's no need to have this function inside a component, and it might end up being a regret in the long-run. we can pass it in via mapStateToProps or better yet, pass in the tag names separately. that would leave this component quite straight-forward and remove the need to jump back and forth between the component and the wrapper to figure out what to send it.
type StateProps = {
tagNames: String[];
}
const mapStateToProps = (state, props) => ae
…
return {
noteId,
note,
tagNames: selectors.noteTags(state, note)
}
}then we add this selector
export const noteTags = (state, note) =>
(note?.tags ?? [])
.filter(tagName => !isEmailTag(tagName))
map(tagName => canonicalTagName(state, tagName));
export const canonicalTagName = (state, tagName) =>
state.data.tags.get(tagHashOf(tagName)).name;a couple of steps back though I think this is pointing to a weakness in our model where we have to do any of this. we could/should consider transforming this when we load the note into state so we don't have to recalculate it every time.
There was a problem hiding this comment.
there's no need to have this function inside a component, and it might end up being a regret in the long-run. we can pass it in via mapStateToProps or better yet, pass in the tag names separately. that would leave this component quite straight-forward and remove the need to jump back and forth between the component and the wrapper to figure out what to send it.
I totally agree, the tag names should be calculated through selectors instead within the component. I'll update this in the TagField component too.
a couple of steps back though I think this is pointing to a weakness in our model where we have to do any of this. we could/should consider transforming this when we load the note into state so we don't have to recalculate it every time.
Do you mean that we should calculate the canonical tag names on the fly when a note is loaded?
I'm a bit confused with the use of hashes for tags, if we use the hash of the tag as the id, shouldn't we use that id in the notes to reference the tags?
There was a problem hiding this comment.
Do you mean that we should calculate the canonical tag names on the fly when a note is loaded?
yes but I didn't mean to imply we have to do it in this PR - more of a note to us all that we're straining under this mismatch between our internal model and the external model of notes and tags. granted, tags have always been weird.
I'm a bit confused with the use of hashes for tags, if we use the hash of the tag as the id, shouldn't we use that id in the notes to reference the tags?
Tags are weird 🤷♂️. The easy direction to convert is from name to hash and so there's not fundamental challenge to storing the name in the note that refers back to a tag hash. If we store the tag hash in the note itself that could end up causing unexpected "garbage" for the note model outside of Simplenote, even popping up in places like exports if we're not careful. We made the decision that we want to display the canonical names for tags in the UI but there's a fundamental recognition we have to hold that it's possible that we get out of sync with that in the note, so a best-effort is made to store the most-recently-known canonical tag name in the note list.
We have made some attempts to give tags an actual name-agnostic id and there were other challenges there. For now the tag hash has resolved the worst offenses, including things like having duplicate-but-separate tags.
There was a problem hiding this comment.
yes but I didn't mean to imply we have to do it in this PR - more of a note to us all that we're straining under this mismatch between our internal model and the external model of notes and tags. granted, tags have always been weird.
Ok, makes sense, actually I wasn't sure if this was something that could potentially be done in this PR so thanks for clarifying it 🙇 .
Tags are weird 🤷♂️. The easy direction to convert is from name to hash and so there's not fundamental challenge to storing the name in the note that refers back to a tag hash. If we store the tag hash in the note itself that could end up causing unexpected "garbage" for the note model outside of Simplenote, even popping up in places like exports if we're not careful. We made the decision that we want to display the canonical names for tags in the UI but there's a fundamental recognition we have to hold that it's possible that we get out of sync with that in the note, so a best-effort is made to store the most-recently-known canonical tag name in the note list.
We have made some attempts to give tags an actual name-agnostic id and there were other challenges there. For now the tag hash has resolved the worst offenses, including things like having duplicate-but-separate tags.
Tags can be tricky to manage for sure, thank you very much for the explanation because it helped me to better understand how they work 👍 .
A name-agnostic id is the first thing that came to my mind when I was reviewing this part, however since tag names should be unique probably it's not a trivial thing to incorporate it.
For now my solution has been to add a selector similar to what you posted in the previous comment, this way at least we won't have duplicated code and besides it simplifies the rendering of the tags (commit).
|
I realised that note restoration doesn't recover deleted tags that were previously assigned to a note. I think it makes sense that if a note is restored to a point where it has a deleted tag, those tags should be also restored. Another option would be to consider that deleted tags shouldn't be restored, this way when moving the history seeker those notes won't show up. I'm currently leaning on the option of restoring deleted tags but I'd appreciate your feedback on this 🙇 . |
This is something we should be able to handle in the data middleware probably, but possibly in the Simperium middleware. |
Yeah, I'm finally handling this in both places. In the |
|
Regarding the deleted tags, I thought that it would be interesting to differentiate them from the rest when they’re displayed in the note's history. This way, we remark somehow to the user that those notes will be restored with the note as well. As a first approach, I added an icon and changed the opacity of the chip to 75% (you can see how it looks like the following screenshot).
As a fallback, we could always display them as regular tags, since this is just a visual improvement, but before taking a decision, I'd appreciate your feedback on this 🙇. |
@sandymcfadden I fixed the a11y issue you mentioned in this commit. |
| tagName, | ||
| }) => ( | ||
| <div | ||
| className={classNames('tag-chip', { selected })} | ||
| className={classNames('tag-chip', { selected, interactive, deleted })} |
There was a problem hiding this comment.
is it truly "deleted" or just something like transitory or temporary or not current? I'm thinking that deleted might suggest that the tag itself was deleted vs. removed from this particular note. maybe that's not the case. tags are confusing.
There was a problem hiding this comment.
The deleted state is used for those tags that are assigned to a note but don't exist in the global state (most likely because they were deleted). This case would only happen when displaying revisions of a note, they might have tags that were deleted.
There was a problem hiding this comment.
oh interesting! I didn't understand that from the code.
There was a problem hiding this comment.
Perhaps I should use different naming if it's a bit misleading 🤔 .
Regarding my previous comment, you can check the code related to how deleted tags are calculated here.
lib/state/data/middleware.ts
Outdated
| .filter((tagHash) => !state.data.tags.has(tagHash)); | ||
| if (tagsToRestore.length > 0) { | ||
| store.dispatch({ type: 'RESTORE_TAGS', tags: tagsToRestore }); | ||
| } |
There was a problem hiding this comment.
we shouldn't need to create a new action for this as we can just as easily watch for this RESTORE_NOTE_REVISION action in the tag reducer. currently we're performing tag logic in this place which I don't think is where I'd look for tag changes, plus we're introducing multiple dispatches when one would do.
put another way we've created two places to do things that belong together, so we can just move all of it to the existing tag reducer and avoid creating the new action type.
There was a problem hiding this comment.
My first approach was to create a new action just in case we might need it in other places but I totally agree that it's not really necessary because it belongs to notes restoration. I'm going to move the tags restoration logic to tags reducer, thanks for pointing me this out, now the code looks cleaner 🙇 .
I've pushed this change in this commit.
There was a problem hiding this comment.
Tag list calculation on middleware
Based on this comment, the tag list calculation is now a bit more complex since it also requires to modify the note tags.
I thought about doing it in the notes reducer, but combineReducers only exposes the notes slice on that reducer. One option would be to create a cross slice reducer that receives both states (notes + tags) or even the full one for this action.
Another alternative I was thinking is to calculate this in the action creator by getting the state (something similar to a thunk action).
I checked the contributing guide but doesn't cover this case, @dmsnell I'd appreciate you feedback on this 🙇 .
Rules for calculating the tag list
When restoring a revision, the tag list has to be calculated having the following rules in mind:
- New email tags shouldn't be included because they might grant undesired users permission.
- Email tags of current note should be kept to prevent modify user permissions, therefore all of them have to be included.
- Depending on user selection, tags that were previously deleted could be included.
APPLY_NOTE_REVISION action
RESTORE_NOTE_REVISION has the note argument that although it's mandatory, it's not being used when creating the action but populated in the middleware. This generates a Typescript error on the code that dispatches this action from revision-selector.
For this reason, I created a new action APPLY_NOTE_REVISION that is dispatched in the middleware after processing RESTORE_NOTE_REVISION, which is not dispatched.
There was a problem hiding this comment.
I created a couple of PRs to explain further the alternatives I mentioned regarding the tag list calculation on the data middleware.
I thought about doing it in the notes reducer, but combineReducers only exposes the notes slice on that reducer. One option would be to create a cross slice reducer that receives both states (notes + tags) or even the full one for this action.
Another alternative I was thinking is to calculate this in the action creator by getting the state (something similar to a thunk action).
There was a problem hiding this comment.
just in case we might need it in other places
I want to reflect on this and bring it back to the heart of what the actions are intended to represent: something that the customer wants to do. Unless we were to open up restoring deleted tags there's not "in case" for this, because the action is restoring a note and the fact that we're restoring tags is a reaction to that intention.
the tag list calculation is now a bit more complex since it also requires to modify the note tags.
we might be conflating vocabulary. I think you are talking about the tag list at the bottom of the note but there's also a major layout piece of the app called "the tag list" in the sidebar. can you explain more where you're having to modify the note tags? what about the Note revision passed in as the revision through OwnProps? aren't the tags already on that object?
I thought about doing it in the notes reducer, but combineReducers only exposes the notes slice on that reducer.
This is a good thing and central to the design of Redux. Each data type operates independently from each other. It's a quick trip to a nightmare when we start spreading around the governance of data outside of itself, vs. being able to jump to the account's tags and ask, "how can these possibly be altered?"
I checked the contributing guide but doesn't cover this case
the reason it's not covered directly is that we want to avoid the kind of patterns that break out of the core mental model. I can look into making that clearer in the document; thank you for reading it! 😄
there may not actually be much of a conundrum or reason to reach for cross-slice reducers or action thunks, as the data itself doesn't imply the kinds of dependencies these code structures would create. to be clear I'm not opposed to having reducers that stem from multiple inputs, but so far I have tried to model state after that inherent dependency vs. passing multiple bits of state into the reducer (which if we were to do that we'd want to pass the previous state in and not the partially-updated state). see the ghosts reducer, for instance, which maintains two separate but dependent values, which by construction will always update atomically together (avoiding data races and consistency issues).
it's not being used when creating the action but populated in the middleware. This generates a Typescript error
that's a bug in the code that we could fix. IIRC that's from early in the rewrite when we were still deciding on the style we'd use for modifying actions and I probably took a pragmatic decision to introduce the type error and lean on the middleware vs. creating a separate action. at this point I'd say we look into following the example of CREATE_NOTE and CREATE_NOTE_WITH_ID. we could create something like RESTORE_NOTE_REVISION_WITH_ID and humorously convert the other direction.
what we may find, however, could be simpler. with the changes in this PR it might be clearer that in our case we truly are tracking the revision in the UI and basing our decisions on that happen-to-be instance, particularly with tags since those deleted tags no longer exist in state. what we might end up doing can skirt all of the concerns just by passing note, which we apparently need to have in order to view the revision in the first place. we generally try to avoid passing objects around when we can pass ids or references instead, but revisions are definitely a different use case and require a bit more ad-hoc data. can you try passing note to restoreRevision and see if that makes all these questions disappear?
thanks for exploring in those two PRs. I wouldn't say I'm thrilled about either of those approaches and so I'd like to keep our focus here and on the heart of the problem: our UI has changed the needs on the data we have previously been passing around - let's match models so the friction is gone.
this is really hard stuff and you're tackling it right away in this repo, bravo. not sure if you have much experience with Redux + React but you're doing a fine job. thank you very much for your effort.
There was a problem hiding this comment.
I want to reflect on this and bring it back to the heart of what the actions are intended to represent: something that the customer wants to do. Unless we were to open up restoring deleted tags there's not "in case" for this, because the action is restoring a note and the fact that we're restoring tags is a reaction to that intention.
Ok, makes sense 👍 .
we might be conflating vocabulary. I think you are talking about the tag list at the bottom of the note but there's also a major layout piece of the app called "the tag list" in the sidebar. can you explain more where you're having to modify the note tags? what about the Note revision passed in as the revision through OwnProps? aren't the tags already on that object?
Yeah sorry for the misleading, when I referenced "the tags list", I meant the tags of the note's revision to restore.
The problem is that we can't pass the revision object as it's in the state when restoring a revision, the reason is that some of the tags, like the email ones, shouldn't be restored because it might add undesired collaborators (this problem was discussed internally p1617789042044500-slack-C036Y8QL4).
This is a good thing and central to the design of Redux. Each data type operates independently from each other. It's a quick trip to a nightmare when we start spreading around the governance of data outside of itself, vs. being able to jump to the account's tags and ask, "how can these possibly be altered?"
I agree and actually I like the approach of maintaining the reducers totally isolated from the rest. I have experience from previous projects where I used Redux and I can truly recall the nightmare that can turn out if the Redux structure (Actions/Middleware/Reducers) is not properly defined and enforced.
the reason it's not covered directly is that we want to avoid the kind of patterns that break out of the core mental model. I can look into making that clearer in the document; thank you for reading it! 😄
No problem 😄 ! I think it would be awesome if we cover this case, at least exposing the pattern we want to follow when we need to trigger an action that requires different slices from the state.
there may not actually be much of a conundrum or reason to reach for cross-slice reducers or action thunks, as the data itself doesn't imply the kinds of dependencies these code structures would create. to be clear I'm not opposed to having reducers that stem from multiple inputs, but so far I have tried to model state after that inherent dependency vs. passing multiple bits of state into the reducer (which if we were to do that we'd want to pass the previous state in and not the partially-updated state). see the ghosts reducer, for instance, which maintains two separate but dependent values, which by construction will always update atomically together (avoiding data races and consistency issues).
One of the powerful features of Redux is that it's quite open so it can be used in different ways. Definitely there're common patterns and good practices but the project's architecture should be the one that drives the use of Redux, for this case makes sense to use it the way you described.
that's a bug in the code that we could fix. IIRC that's from early in the rewrite when we were still deciding on the style we'd use for modifying actions and I probably took a pragmatic decision to introduce the type error and lean on the middleware vs. creating a separate action. at this point I'd say we look into following the example of CREATE_NOTE and CREATE_NOTE_WITH_ID. we could create something like RESTORE_NOTE_REVISION_WITH_ID and humorously convert the other direction.
Yep, I thought about doing something similar to note creation logic. A new action like what you mentioned (RESTORE_NOTE_REVISION_WITH_ID) would work for this case, however I decided to continue with the option described at the bottom of the comment, that doesn't require to create this new action ⬇️ .
what we may find, however, could be simpler. with the changes in this PR it might be clearer that in our case we truly are tracking the revision in the UI and basing our decisions on that happen-to-be instance, particularly with tags since those deleted tags no longer exist in state. what we might end up doing can skirt all of the concerns just by passing note, which we apparently need to have in order to view the revision in the first place. we generally try to avoid passing objects around when we can pass ids or references instead, but revisions are definitely a different use case and require a bit more ad-hoc data. can you try passing note to restoreRevision and see if that makes all these questions disappear?
No problem, for this option we would need to build the revision object before dispatching the object.
After exploring the different options, I think that in the end the solution is related to where it makes more sense to handle the revision object build. Here I described briefly the options I see:
- Reducer: This would require a cross slice reducers because we need data from different slices. As commented earlier, this is discarded in favor of keeping the reducers operating independently from each other.
- Middleware: This is where we're currently handling the logic when we need to transform data upon action execution, as an example we have the create note action. I think we could keep using it for most cases, nevertheless following your comment I tried the fourth option described below.
- Action creators: Unfortunately we don't have access to the state when creating an action, however this could be solved by adding thunk actions. From my experience, this looks like the most common solution but it's true that it might not fit all projects like this one and besides it comes with its tradeoffs.
- Selectors: This would be used in the UI before dispatching an action, in this case I think this seems to be the best option since we need to build the revision object depending on UI like including the deleted tags.
thanks for exploring in those two PRs. I wouldn't say I'm thrilled about either of those approaches and so I'd like to keep our focus here and on the heart of the problem: our UI has changed the needs on the data we have previously been passing around - let's match models so the friction is gone.
this is really hard stuff and you're tackling it right away in this repo, bravo. not sure if you have much experience with Redux + React but you're doing a fine job. thank you very much for your effort.
Thanks 😊 ! Thank you very much for all the feedback!
I used Redux on some React Native projects in the past and I know that it can be tricky. Redux is a common discussion topic and it usually raises discussions about how to use it, but I always found them very interesting and useful. From my experience one of the most important things regarding this topic is to have as clear as possible architecture, otherwise it's quite easy to lose control.
Conclusion
Following your comments, I decided to try to pass the note in the RESTORE_NOTE_REVISION action in this commit. For this option, I created a new selector that creates the revision object and it's used in the revision-selector component. This way when we dispatch the restore revision note action we pass the final object to be included straight to the state.
lib/tag-field/index.tsx
Outdated
| @@ -25,10 +26,10 @@ type OwnState = { | |||
| }; | |||
|
|
|||
| type StateProps = { | |||
| allTags: Map<T.TagHash, T.Tag>; | |||
| tags: Map<T.TagHash, T.Tag>; | |||
There was a problem hiding this comment.
it doesn't look like we intrinsically need the hash here. would it make sense to return a simpler data structure and make a simpler selector to return T.TagName[] instead of the map from tag hash to tag record?
There was a problem hiding this comment.
The tag's hash is being used as the key prop for rendering the tag chips but we could use the tag's name instead, in this case it would be enough with T.TagName[]. However since the noteTags selector is also calculating if a tag is deleted, I'd keep returning T.Tag object because we need it for note-revisions component.
One thing that we could change is that instead of returning a Map, we could simply return an array of T.Tag. You can check this change in this commit.
I think this makes sense too! But I wonder if users should be given the choice of whether to restore them or not.
I don't think there is an adequate only-visual indicator that can explain a concept like "these tags will appear in your tags list again after restoring the note". Just to throw a few ideas out there:
Let me know what you think. @codebykat @sandymcfadden Any thoughts on this one? |
| if (!prevState.data.tags.has(tagHash)) { | ||
| queueTagUpdate(tagHash, 10); | ||
| } | ||
| }); |
There was a problem hiding this comment.
this could resolve a lot of issues we currently have with tags not syncing properly
|
Heads up that I'm going to explore the second option from @SylvesterWilmott comment related to adding a checkbox for let the user decide if tags should be restored along with the revision. Additionally, as discussed internally in p1617789042044500-slack-C036Y8QL4, I'm also going to prevent the email tags to be restored. @dmsnell @sandymcfadden I'll let you know when I apply these changes to continue PR review. Thank you very much for all the help you’re bringing to me along reviews 🙇 . |
| } | ||
|
|
||
| &.deleted { | ||
| background: $studio-red-5; |
There was a problem hiding this comment.
@SylvesterWilmott based on the screenshots you shared, I used this color for deleted tags in light theme, let me know if I should use a different one.
Screenshot
| @@ -46,6 +53,10 @@ | |||
| .tag-chip { | |||
| background: $studio-gray-70; | |||
| color: $studio-white; | |||
|
|
|||
| &.deleted { | |||
| background: $studio-red-70; | |||
There was a problem hiding this comment.
@SylvesterWilmott For the dark theme, following the design guidelines, I used the dark version of the same color, let me know if I should use a different one.
Screenshot
lib/note-revisions/index.tsx
Outdated
| const tags = noteTags(state, note).filter( | ||
| (tag) => showDeletedTags || !tag.deleted | ||
| ); |
There was a problem hiding this comment.
Depending on showDeletedTags value, the deleted tags are filtered out and not displayed.
lib/revision-selector/index.tsx
Outdated
| <div className="revision-deleted-tags-toggle"> | ||
| <ToggleControl | ||
| id="revision-deleted-tags-checkbox" | ||
| checked={showDeletedTags} | ||
| onChange={toggleDeletedTags} | ||
| /> | ||
| <label | ||
| htmlFor="revision-deleted-tags-checkbox" | ||
| className="revision-deleted-tags-toggle-label" | ||
| > | ||
| Restore deleted tags | ||
| </label> | ||
| </div> |
There was a problem hiding this comment.
This parts adds the toggle control as you can see in the following screenshots:
Light theme
| Untoggled | Toggled |
|---|---|
 |
 |
Dark theme
| Untoggled | Toggled |
|---|---|
 |
 |
There was a problem hiding this comment.
gosh this stuff looks so good. thanks for the persistence on the issue!
The PR is ready to review, I've made some changes regarding the tag list calculation and I introduced a toggle for enable/disable the deleted tags when restoring a revision. Let me know if you have any doubt, thank you very much for your help 😊 ! |
|
Looking good @fluiddot ! In general, I think toggles have a tough time communicating whether something is a state or an action. For example, it's possible to assume that by moving the toggle to the on position that deleted tags will be immediately restored. I think in this case it would be better to use a checkbox.
Looks good to me! |
When restoring a note revision, email tags are included so we should display them too in the note's history.
When restoring a revision, the tag list has to be calculated having the following rules in mind: 1. New email tags shouldn't be included because they might grant undesired users permission. 2. Email tags of current note should be kept to prevent modify user permissions, therefore all of them have to be included. 3. Depending on user selection, tags that were previously deleted could be included.
The noteTags selector is already filtering them so it's not needed.
This selector will return the revision object to be passed to the restore note revision Redux action.
noteTags selector has been refactored due to this change, now it only returns the canonical tag names.
fluiddot
force-pushed
the
add/tags-history
branch
from
8d21fc9 to
f8ad09e
Compare
|
👋 @dmsnell @sandymcfadden The PR is ready for a final review 🎊 , let me know your feedback and if there's anything to update/fix I'll be more than happy to apply it. Thanks for your help in this feature 🙇 and sorry for ending up with such a big PR 😄 ! |
lib/note-revisions/index.tsx
Outdated
| return ( | ||
| <div aria-hidden={ariaHidden} className="note-revisions"> | ||
| <NotePreview noteId={noteId} note={note} /> | ||
| <div className="tags"> |
There was a problem hiding this comment.
can we double-check the style here with tags as a bare value. I'm not as familiar with the rest of the CSS in this app but I do know we generally like to avoid such generic names as well as heavy nesting in SCSS (again I say I can't remember off-hand what our norms are in this repo).
might be valuable in any case to leave a more specific name here since tags is everywhere. something like note-tag-list possibly?
There was a problem hiding this comment.
Oh, good catch, I do agree that this class name is too generic. I've just renamed it in this commit, thanks!
| noteId, | ||
| note, | ||
| }; | ||
| }; |
There was a problem hiding this comment.
based on our current use of the revisions screen I think this is fine, but I did want to point out that this triggers a re-render unconditionally because of how we're creating new copies of the lists in mapStateToProps.
if in the future we want to address this, or if we end up with glitchy rendering, we can memoize it here or as a selector, or since this is all relatively stateless computation we can move that into the component itself where it will only run when the props change vs. on every update to the app, even unrelated updates. well, I guess noteCanonicalTags is creating a new copy as well - eventually we might want to look at that.
this is for information purposes only and to leave a trace of the conversation; it's not a blocker for this PR
Nice! Thanks for the final review @dmsnell 🙇 ! @sandymcfadden since this is a major change I'd like to have both approvals if possible, let me know your opinion, if you think it's ok too, I'll go ahead and merge it. |
|
I also did another round of testing and everything works as expected so I'm going to merge it 🎊 ! Thanks @dmsnell and @sandymcfadden for the help on this PR! |
Fix
Fixes #2654
In the note's history screen, currently only the note's content of the selected revision is displayed, this PR adds the tags so users can now see the tags history too.
tags-history.mp4
Test
Notes
simplenote_v2(located in developer tools:Application/IndexedDB/simplenote_v2) and refresh client.Restore a note
Restore a note with deleted tags
Restore a note not including deleted tags
Prevent email tags restoration
Prevent system tags restoration
Release
Add note's tags to the history screen.