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

PROD-1685 show warning in history if space deleted #547

Merged
merged 6 commits into from
Apr 3, 2020
Merged

Conversation

mason-fish
Copy link
Contributor

Shows a warning ⚠️ symbol next to history entries which no longer have their corresponding space. Related work that I've included here is importing the react-tooltip component as well as making sure that the tree logic for loading past history items behaves the same way that linear history does. Previously it would only load the search entry in whatever tab/space you already have open, and throw an error if nothing was open yet.

historywarning

@mason-fish mason-fish requested review from jameskerr and a team April 2, 2020 01:38
Copy link
Member

@jameskerr jameskerr left a comment

Choose a reason for hiding this comment

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

Thanks Mason. A few suggested changes:

  1. The styles that are being applied to the svg icon are not specific enough and are being applied to the other svg (the close X on the tree view). By the way, this may have already been in master, but I figure here is a good place to fix it.

Screen Shot 2020-04-03 at 9 58 11 AM

That margin right is causing the x to be misplaced in the tree view:

Screen Shot 2020-04-03 at 9 53 53 AM

  1. Add a class to the component that is wrapping the icon (that will allow you to target just that svg and fix the above error).

  2. Try to make the icon sit vertically centered with the other elements in the row.

To do this, make sure that the parent elements all have display: flex; align-items: center and so does the class that you give to the element that wraps the icon.

It looks like some of the elements have a margin-bottom: 3px. You may need to add that to the icon-wrapper to get them centered vertically.

  1. I think the icon looks a little big right now. Maybe 11px height and width?

Screen Shot 2020-04-03 at 9 57 21 AM

</div>
)

if (!includes(spaces, findingSpace)) return body
Copy link
Member

Choose a reason for hiding this comment

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

Why not move this check to right under line 59 and return null early before creating the body variable. A few less CPU cycles.

@@ -130,6 +130,7 @@
"react-measure": "^2.3.0",
"react-redux": "^7.1.3",
"react-spring": "^8.0.27",
"react-tooltip": "^4.1.3",
Copy link
Member

Choose a reason for hiding this comment

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

🎉

@@ -130,6 +130,7 @@
"react-measure": "^2.3.0",
"react-redux": "^7.1.3",
"react-spring": "^8.0.27",
"react-tooltip": "^4.1.3",
Copy link
Member

Choose a reason for hiding this comment

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

🎉

Mason Fish added 6 commits April 3, 2020 11:36
Signed-off-by: Mason Fish <mason@looky.cloud>
Signed-off-by: Mason Fish <mason@looky.cloud>
Signed-off-by: Mason Fish <mason@looky.cloud>
Signed-off-by: Mason Fish <mason@looky.cloud>
Signed-off-by: Mason Fish <mason@looky.cloud>
Signed-off-by: Mason Fish <mason@looky.cloud>
Copy link
Member

@jameskerr jameskerr 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!

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.

2 participants