[#1526] Add ellipsis to sidebar file names #1527
Conversation
| @@ -10,6 +10,47 @@ import FolderRightIcon from '../../../images/triangle-arrow-right.svg'; | |||
| import FolderDownIcon from '../../../images/triangle-arrow-down.svg'; | |||
| import FileIcon from '../../../images/file.svg'; | |||
|
|
|||
| function FileName({ name }) { | |||
| const nameArray = name.split('.'); | |||
There was a problem hiding this comment.
It's a good idea to catch if (!name) return; at the beginning of the function, otherwise .split can explode
There was a problem hiding this comment.
name is a required string proptype, which means that the component itself will complain if it's used without this prop, so is this necessary?
There was a problem hiding this comment.
True, in this case it's kind of pointless
| const extension = `.${nameArray[nameArray.length - 1]}`; | ||
| const fileName = nameArray.slice(0, -1).join(''); | ||
| const firstLetter = fileName[0]; | ||
| const lastLetter = fileName[fileName.length - 1]; |
| function FileName({ name }) { | ||
| const nameArray = name.split('.'); | ||
| if (nameArray.length > 1) { | ||
| const extension = `.${nameArray[nameArray.length - 1]}`; |
There was a problem hiding this comment.
You can use nameArray.slice(-1) here, if that's more readable
There was a problem hiding this comment.
I always forget that I can use slice for this 😄
There was a problem hiding this comment.
I think I'm actually going to change this to nameArray.charAt(nameArray.length - 1) as I think that's the most readable.
There was a problem hiding this comment.
Wow, so apparently charAt can work in some really wacky ways (see this StackOverflow answer), so......... I'm just going to stick with bracket notation.
There was a problem hiding this comment.
Make sure to check/test what happens if you pass an empty string to it!
Javascript doesn't like array[-1]
There was a problem hiding this comment.
Nevermind, you're catching this on line 15. My bad!
| return ( | ||
| <span className="sidebar__file-item-name-text"> | ||
| <span>{firstLetter}</span> | ||
| {fileName.length > 2 && | ||
| <span className="sidebar__file-item-name--ellipsis">{middleText}</span> | ||
| } | ||
| {fileName.length > 1 && | ||
| <span>{lastLetter}</span> | ||
| } | ||
| <span>{extension}</span> | ||
| </span> | ||
| ); |
There was a problem hiding this comment.
If possible, I think FileName should be just this block, and the name logic should go on another function, which returns a string. This will avoid having to duplicate this code below.
| ); | ||
| } | ||
| const firstLetter = name[0]; | ||
| const lastLetter = name[name.length - 1]; |
| return ( | ||
| <span className="sidebar__file-item-name-text"> | ||
| <span>{firstLetter}</span> | ||
| {name.length > 2 && | ||
| <span className="sidebar__file-item-name--ellipsis">{middleText}</span> | ||
| } | ||
| {name.length > 1 && | ||
| <span>{lastLetter}</span> | ||
| } | ||
| </span> | ||
| ); |
- Create function parseFileName to separate a file name into first letter, last letter, middle, extension
…js-web-editor into bug/sidebar-name-overflow
|
Other things I still need to consider for this:
|
|
Maybe, if there's already a concrete improvement delivered by this PR, you can address these concerns later in another issue/PR. |
I'm pretty sure this is broken by this PR, so I want to fix it.
This, however, is something I think could be addressed in a different PR! |
|
Tested this with VoiceOver, file labels are working 😄 |
Fixes #1526
I have verified that this pull request:
npm run lint)Fixes #123I went a little overboard and decided to copy Glitch's design on this one—would love some feedback!
