-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Remove searched by username #13373
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
Remove searched by username #13373
Conversation
modified: ui/analyse/src/explorer/explorerConfig.ts
|
I didn't find a way to add to #12885 so I opened a new pr with my commits |
this is a complete rewrite of the UI to get rid of the gap that appeared because if moved the x with css
| } | ||
| .previous { | ||
| @extend %flex-wrap; | ||
| grid-template-columns: auto auto; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't that for grid elements? what sense does it make in a flex context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it allows me to use column-gap to increase the gap between the elements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you use gap instead for flex elements
| width: 1.75em; | ||
| height: 1.75em; | ||
| top: -1.5em; | ||
| right: -0.65em; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
margin-left: -1.75em;
width: 1.75em;
height: 1.75em;
top: -1.5em;
right: -0.65em;that looks extremely fragile :-/ is there a better way that doesn't require 5 constants to maintain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so, the margin left is required to close the gap, width and height aren't necessary but sol pointed out in the discord that the circle around the cross was too small so I had to increase width and height and top and right are required to move it into the corner what might be possible is to calculate everything from 1 value in this case 1.75, I'll be looking into that
| height: 1.75em; | ||
| top: -1.5em; | ||
| right: -0.65em; | ||
| background-color: rgb(60, 60, 50); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use the existing c-*** colors, and see how mix is used in our scss files to create new colors from them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't find the c-*** color for that gray
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be $c-bg-popup.
| removePlayer = (name?: string) => { | ||
| if (!name) return; | ||
| const previous = this.data.playerName.previous().filter(n => n !== name); | ||
| this.data.playerName.previous(previous.slice(0, 20)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
slice is not useful here
|
@DrSteinbot Please implement ornicar's fixes or let us know if you're unavailable to work on this fix at the moment. |
|
Or set as in progress (draft) |
|
I decided to test out the fix... When there are multiple names, the buttons (both the blue squares and the delete buttons) to the right of the removed name are no longer aligned to the actual names (they reflect the original order instead). (To fix, set the vnode key to the name of the player) |
|
@DrSteinbot, I've had a working version of this fix, with ornicar's comments addressed, on my computer for several days. Would you mind if I opened up a new pull request for this? |
|
No not at all, though I was looking forward to continuing development tomorrow after being away from my dev machine for 3 weeks, if you already have a working version submit it |
|
Awesome. I'll submit it. |
|
closing in favor of #13518 |
No description provided.