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.

Sign up for GitHub

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

Jump to bottom

Feature/button-to-close-flow-details-view #6734

Merged
merged 13 commits into from Mar 25, 2024
Merged

Feature/button-to-close-flow-details-view #6734

merged 13 commits into from Mar 25, 2024

Conversation

lups2000
Copy link
Contributor

@lups2000 lups2000 commented Mar 13, 2024 •
edited

Description

Added a button to close the flow details panel, as discussed in issue #6725. It could be useful to have a button to immediately close the section. This is a preliminary idea, as I didn't want to alter the design significantly. What are your thoughts?

Screen.Recording.2024-03-14.at.17.49.00.mov

Checklist

  • I have updated tests where applicable.
  • I have added an entry to the CHANGELOG.

@mhils
Copy link
Member

mhils commented Mar 14, 2024

Code looks good, can you include a rendered preview in the PR description please? 😃

@lups2000
Copy link
Contributor Author

lups2000 commented Mar 14, 2024 •
edited

yes I'm uploading a screen recording :) Let me know if I have to change something! Thanks

@Prinzhorn
Copy link
Member

Tiny nitpick: please make this a <button>

@lups2000
Copy link
Contributor Author

lups2000 commented Mar 14, 2024 •
edited

Tiny nitpick: please make this a <button>

should we keep the default design of a <button>? it was not the best next to the tabs

@Prinzhorn
Copy link
Member

should we keep the default design of a <button>? it was not the best next to the tabs

No, but <span> is not an interactive element.

These are all <button>:

image
image

@lups2000
Copy link
Contributor Author

should we keep the default design of a <button>? it was not the best next to the tabs

No, but <span> is not an interactive element.

These are all <button>:

image image

Okay I see, I'll fix that :)

@mhils
Copy link
Member

mhils commented Mar 15, 2024

Thanks. This does provide the functionality we're after, but doesn't look polished yet. Can we make it an "X" that is either right-aligned or that is very first item?

For inspiration, see what the Chrome Devtools are doing:

image

Having it on the left (as in the screenshot) is maybe easiest, in particular if ever plan to implement better overflow support.

@lups2000
Copy link
Contributor Author

Thanks. This does provide the functionality we're after, but doesn't look polished yet. Can we make it an "X" that is either right-aligned or that is very first item?

For inspiration, see what the Chrome Devtools are doing:

image Having it on the left (as in the screenshot) is maybe easiest, in particular if ever plan to implement better overflow support.

sure! I'll try to put in as a first item and see the results

@lups2000
Copy link
Contributor Author

@mhils what do you think now? Should I change the color ?
Screenshot 2024-03-15 at 13 34 55

@mhils
Copy link
Member

mhils commented Mar 15, 2024

Nice! I don't think it needs emphasis, so probably just gray? It would also be cool to use a proper close icon and not just a text x. Does our fontawesome version have xmark yet? :)

@lups2000
Copy link
Contributor Author

lups2000 commented Mar 15, 2024 •
edited

Nice! I don't think it needs emphasis, so probably just gray? It would also be cool to use a proper close icon and not just a text x. Does our fontawesome version have xmark yet? :)

okay perfect! Apparently xmark is not listed in the font-awesome.css. If we want to add it, we should probably download the new files from here: https://fontawesome.com/download , right?

@mhils
Copy link
Member

mhils commented Mar 15, 2024

Upgrading to fontawesome v5 isn't entirely painless here I'm afraid. Let's not add feature creep to this PR. Can we replace the icon for abort with fa-times-circle, and use fa-times for the close button?

@lups2000
Copy link
Contributor Author

lups2000 commented Mar 15, 2024 •
edited

done :)

@lups2000
Copy link
Contributor Author

Should I add something else? Or is it fine now? @mhils

@mhils
Copy link
Member

mhils commented Mar 22, 2024

Could you post a screenshot of what it looks like now please?

@lups2000
Copy link
Contributor Author

lups2000 commented Mar 22, 2024 via email

@lups2000
Copy link
Contributor Author

Screenshot 2024-03-22 at 20 27 06

@mhils

mhils
mhils reviewed Mar 25, 2024
View reviewed changes
Copy link
Member

@mhils mhils left a comment

Choose a reason for hiding this comment

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

Thanks, this looks great now! Maybe add color: black on hover?

web/src/css/flowdetail.less Outdated
@@ -21,6 +21,17 @@
}
}

.close-button {
margin-top: 2;
Copy link
Member

Choose a reason for hiding this comment

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

Some leftovers here due to this now being a button?

(I also cannot resist: Two what? Two horses?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, to align the icon with the text of the tab

mhils
mhils reviewed Mar 25, 2024
View reviewed changes
web/src/css/flowdetail.less
font-size: 15px;
cursor: pointer;
border: none;
}
Copy link
Member

Choose a reason for hiding this comment

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

There are still some leftovers here. For example, cursor: pointer; is unnecessary for a button.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, just cursor and font-weight, the others are necessary I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

now?

@lups2000
Copy link
Contributor Author

wait why the check fails now? I just removed 2 lines 😅

@mhils mhils merged commit e834259 into mitmproxy:main Mar 25, 2024
22 checks passed
@mhils
Copy link
Member

mhils commented Mar 25, 2024

Thanks! 🍰

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhils mhils mhils approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@lups2000 @mhils @Prinzhorn