-
Notifications
You must be signed in to change notification settings - Fork 188
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
Height reduction of highlight #2111
Height reduction of highlight #2111
Conversation
As usually, screenshot before/after would be nice. |
Test Results 1 814 files + 1 1 814 suites +1 1h 30m 5s ⏱️ + 1m 16s For more details on these failures, see this check. Results for commit 78a2b7a. ± Comparison against base commit e30a52d. ♻️ This comment has been updated with latest results. |
This change even makes sense without the overall theme changes, doesn't it? So would it be possible to remove the theme change commit from this PR and make it only consist of the hight reduction? We can probably can faster agree on that and merge than on the overall theme changes. |
And can we please add the relevant screenshots here? Its really hard to guess/follow what will exactly be affected here. Also as far as I can see this will affect all user of CTab and is not only a styling change. |
Just tried it out. To get a first impression, find some screenshots below. Looks better in my opinion. The old marker was pretty bold and made the overall header not look layouted correctly (the text looked as if it should be placed more to the top when the marker popped up). Only thing I see is a slight vertical misplacement of the marker (there is 1 pixel white line in the tab header below the marker line). This is better visible when using 200% monitor scaling. |
@HeikoKlare Fully agree with you. We always wanted to do this separately. Regarding the misalignment. Did not notice this before. Looks like we have to push it down by one pixel. What do you think? |
@@ -624,7 +624,7 @@ void drawSelectedTab(int itemIndex, GC gc, Rectangle bounds) { | |||
int verticalOffset = highlightOnTop ? 0 : bounds.height - 2; |
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 the cause for the misalignment is the combination with this magic number used for the vertical placement. Changing it to 1
seems to solve the issue.
I propose to put the highlight thickness into a separate variable and use it here for the offset calculation as well as for the line thickness. That improves modifiability and comprehensibility (like always when facing magic numbers 🙂 ).
Thank you for the update! To me, this looks good now. @mvm-sap could you please squash the commits to a single one. I propose to merge this soon to receive feedback of early adopters that do not take notice of and investigate each PR. Thus, if no one objects, I will merge until end of today. Here is how it looks now, also incorporating the new theme that has just been merged. I see even more value in this change with the new themes, as now every view/editor stack shows a highlight at the selected tab, which makes the existing highlights look even more bulky. |
Looks good to me as well. Yes, once we start using the theme we noticed that the underline is too thick. Looking forward to see how it feels with the smaller one. |
6f410e1
to
78a2b7a
Compare
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.
Thank you for updating the PR! Looks good now. I've just tested again. Once the builds are done, let's merge it, so it might still be part of M2.
Mac failure seems unrelated |
@vogella fyi: this kind of failure is documented here: eclipse-pde/eclipse.pde#1310 |
I played around with the newest iBuild, yesterday. For me this is a change which needs time to get used to. So, I do not want to give an opinion on it, yet. However, while I personally prefer minimalist designs. I am currently not sure if this is pushing it too far to the minimalist side. In the end, form should follow functions and not the other way around. @HannesWell already posted some thoughts that the new tab design makes them less visible (#2114 (comment)). The new tab design is the standard in modern UIs and modern means more minimalist, more focus on whitespacing. But his comment motivated me to look at existing designs for tabs, again. Here a collection of examples which might be helpful for you. Eclipse new light theme before this change (Line-weight: thick, Font-weight: light) Eclipse new light theme with this change (Line-weight: thin, Font-weight: light) Intellij light theme (Line-weight: thick, Font-weight: light) VS Code light theme (Line-weight: thin, Font-weight: medium -> not selected tabs are grey) Microsoft Teams (Line-weight: thick, Font-weight: thick) Microsoft Outlook (Line-weight: thick, Font-weight: thick) Some thoughts
What are your thoughts? |
I personally have the feeling that proposal 2 is a good one. It not only makes the "blue" line feeling thicker but also helps in separating the view tabs from the view content. |
Agree with that. I would also prefer a visual horizontal separation better, especially for inactive tabs. |
Reduced the thickness of the blue line (highlight) for selected tabs.
For the full details of the change including screenshots see issue: #2114
See the highlight for the selected Properties tab