added ens name inside a div #254
Conversation
|
@Sadaf-A can you include at least one screenshot showing that it works please? Try to do this with any UI change PRs if you can. As far as I can tell this does not resolve the issue. This name should be cut to size and have an ellipsis or elegantly wrap without breaking the table.
|
|
Hey, I made some more changes here are the final results. 
|
| @@ -85,6 +85,8 @@ table td div { | |||
| word-break: break-all; | |||
| overflow: hidden; | |||
| text-overflow: ellipsis; | |||
| white-space: nowrap; | |||
| max-width: 200px; | |||
There was a problem hiding this comment.
Why 200px, is this the most optimal width given the available space across mobile and desktop?
There was a problem hiding this comment.
I changed it to 240px here is how it will look on an iPhone SE
I think increasing it further may cause issues
There was a problem hiding this comment.
I do too but I also think this approach isn't very responsive and could be improved to cater for other viewports. While it may also resolve the other overlap I mentioned, it will likely make it harder to effectively size both.
You have the right idea mate it's just the getting the execution right now.
P.S I appreciate the images as will other reviewers but one after each edit is not required, you can keep one or two main images/videos in the first comment up to date when you are satisfied with your changes. The odd image throughout the PR is okay but it makes it easier for other reviewers to quickly grasp where you at with things if it's up the top. Thanks @Sadaf-A
|
@Sadaf-A you have also resolved the additional overlapping issue contained within the |
|
@Sadaf-A could you also try to use conventional commits cheatsheet, it's part of CI and will hold back PRs if CI cannot complete without issues (except under special circumstances) action repo |
Hey, please provide your inputs now |
|
The code works and looks fine. How those changes look aesthetically will need additional confirmation whether they are suitable enough to be merged. Personally I think the space could be better used, what do you think @rndquu? Three viewports showing how changes affect the UI (ens name is pasted 3 times)
|
updated view here |
|
Better use of the space for sure, only one or two viewports does it still happen (Galaxy S8+ It could be tweaked and tweaked but this might be as good as you'll get things unless you handle each of the two areas separately. Because by fixing the I'd suggest to shave off just enough that no overflow happens on any viewport or to handle them separately, thanks @Sadaf-A |
could you please elaborate like what is the PR missing? |
Your solution affects another part of the UI which means you now need to ensure that both parts are error free if you intend on using this solution. See the first quote above for where the error still exists and the second quote for what I suggest to do. |
There was a problem hiding this comment.
- The issue with this approach is that if we change the container width (for example make it bigger) then there will be an empty space where ENS name could be rendered:
Can't we use flexbox without manually setting max-width in the css files?
- It makes sense to be more specific in CSS selectors in order not to break anything. So if you wrap ENS name in a
divthen wrap it in smth like<div class="ens-name">which can be later utilized in css files likediv.ens-name. This way we greatly reduce chances to break UI of other table items.
@Sadaf-A Pls refactor accordingly
Resolves #237
putting the ens name inside of a div utilises the existing css to handle overflowing names