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

Rename tooltip and fixes #912

Merged
merged 4 commits into from
Apr 12, 2021
Merged

Rename tooltip and fixes #912

merged 4 commits into from
Apr 12, 2021

Conversation

tanmoyAtb
Copy link
Contributor

closes #838

@render
Copy link

render bot commented Apr 11, 2021

@render
Copy link

render bot commented Apr 11, 2021

@render
Copy link

render bot commented Apr 11, 2021

@tanmoyAtb tanmoyAtb self-assigned this Apr 11, 2021
@tanmoyAtb tanmoyAtb marked this pull request as ready for review April 12, 2021 14:33
@tanmoyAtb tanmoyAtb added the Status: Review Needed 👀 Added to PRs when they need more review label Apr 12, 2021
@@ -427,7 +431,7 @@ const FileSystemItemRow: React.FC<IFileSystemItemRowProps> = ({
<TableCell
ref={preview}
align="left"
className={classes.filename}
className={clsx(classes.filename, editing && "editing")}
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is something wrong with this editing flag (certainly not from this PR, we can move this to another issue, just mentioning it here in case you know more). When I checked for it, I saw that all the rows actually get the .editing class, even the ones you're no editing. So in my example below, I have a long name for a folder. I edit a file (with a short name) and in the background, you can see the overflow visible, and the scrollbar:|
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, why thats happening. this can be fixed in this PR!
Great catch!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I suspected, having overflow in mobile view was root of the problem, also prevented editing styled to pass into all the rows.
Should be good now.
Thanks for getting to this!

Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
@tanmoyAtb tanmoyAtb added the Status: Changes Requested ❌ Added to PRs that require further changes from the contributor. label Apr 12, 2021
@tanmoyAtb tanmoyAtb removed the Status: Changes Requested ❌ Added to PRs that require further changes from the contributor. label Apr 12, 2021
Copy link
Contributor

@FSM1 FSM1 left a comment

Choose a reason for hiding this comment

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

looks great @tanmoyAtb 💯

Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

Nice, working great now!

@tanmoyAtb tanmoyAtb merged commit ef4366d into dev Apr 12, 2021
@tanmoyAtb tanmoyAtb deleted the feat/rename-tooltip-838 branch April 12, 2021 16:44
FSM1 added a commit that referenced this pull request Apr 12, 2021
Crash when going back after having selected Google auth #860 (PR #871)
Login optimizations (PR #879)
Minor fixes + Updates to color manipulator (#874)
Merge pull request #879 from ChainSafe/fix/login-optimizations
Merge pull request #887 from ChainSafe/fix/tbaut-hover-color
Merge pull request #886 from ChainSafe/feat/tkey-darkmode
Remove code duplication (#889)
Merge pull request #895 from ChainSafe/fix/select-all-892
Merge pull request #896 from ChainSafe/fix/mouse-highlight-issue-890
Merge pull request #894 from ChainSafe/fix/mobile-spinner-893
Merge pull request #877 from ChainSafe/feat/tbaut-settings-security-846
folder validation (#899)
saved browsers settings (#865)
Prevent the whole app from rerendering every 5s (#909)
Merge pull request #911 from ChainSafe/fix/share-transfer-modal-on-ta… …
Merge pull request #905 from ChainSafe/fix/bin-file-view-873 …
Merge pull request #906 from ChainSafe/feat/home-folder-nav-885 …
Merge pull request #907 from ChainSafe/fix/search-bucket-to-reset-on-…
Fix: Search results all show as files (#914) …
Fix double click opening the next file preview (#915) …
Rename tooltip and fixes (#912) …
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Review Needed 👀 Added to PRs when they need more review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename File tooltip is not rendering correctly
3 participants