Sync code hint font size with code view font size #13091

swmitra · 2017-02-10T08:38:36Z

This PR enhances Brackets user experience by syncing the code hint font-size with code view font size. This was a problem in high dpi monitors ( WQHD and above ), where code hints were not readable with the current font-size. With this PR , when user changes code view font size, code hint font size also gets changed accordingly.

Without this PR, code hints are rendered as -

With the changes in PR -

ficristo · 2017-02-10T10:20:20Z

In case of JS hints the icon doesn't look good.

ficristo

I would like to see at least the documentation fixed.

ficristo · 2017-02-10T10:20:48Z

src/view/ViewCommandHandlers.js

@@ -122,10 +128,10 @@ define(function (require, exports, module) {
     * @param {string} value Is the value of the style
     * @param {boolean} important Is a flag to make the style property !important


cssRule and cssText aren't documented.

ficristo · 2017-02-10T10:22:16Z

src/view/ViewCommandHandlers.js

        cssRule = cssRule || ".CodeMirror";
        var $style   = $("<style type='text/css'></style>").attr("id", propertyID);
-        var styleStr = StringUtils.format("{0}: {1}{2}", name, value, important ? " !important" : "");
+        var styleStr = cssText || StringUtils.format("{0}: {1} {2}", name, value, important ? " !important" : "");


I believe you can remove the space between this two parameters {1} {2}.

ficristo · 2017-02-10T10:22:29Z

src/view/ViewCommandHandlers.js

+
+    /**
+     * @private
+     * Adds a new embeded style top sync code-hint font size with codeview font size


fontSize is not documented

ficristo · 2017-02-10T10:26:21Z

src/view/ViewCommandHandlers.js

+        var styleStr = "";
+        styleStr = styleStr + StringUtils.format("{0}: {1} {2};", "font-size", fontSize, " !important");
+        styleStr = styleStr + StringUtils.format("{0}: {1} {2};", "line-height", (parseInt(fontSize, 10) + 2) + fontSize.replace(parseInt(fontSize, 10), ""), " !important");
+        _addDynamicProperty(DYNAMIC_CODEHINT_FONT_STYLE_ID, "font-size", fontSize, true, ".codehint-menu .dropdown-menu li a", styleStr);


You need to pass the values font-size, fontSize and true but they aren't actually used.
Perhaps it's personal style but it doesn't seems good.

I think function _addDynamicProperty(propertyID, options) would make more sense (where options would be an object of { name, value, important, cssRule, cssText } etc.)

swmitra · 2017-02-13T04:56:45Z

Thanks a lot @ficristo for the quick review 👍 I will update the PR soon.

MiguelCastillo · 2017-02-13T19:50:49Z

Question around the approach. Adding the a method specific for code hints seems reasonable, but I am not sure how that will scale if we had to add more methods for different parts of the UI. Is there a way we can leverage CSS for the fonts size/type?

swmitra · 2017-03-07T07:08:10Z

@MiguelCastillo Good point to consider 👍.
I had tried the most generic approach by increasing the base font size for body. In that way we can increase and sync the font size in other panes like project panel and Dialogs as well. But it's not convincing use case for every one. In fact most of the people I spoke with wanted to increase just the code view font size and nothing else. I will hold this PR for now and lets ask other developers about the actual feature scope itself. Please let me know your opinion about the feature scope.

zaggino · 2017-03-07T07:24:21Z

I'd say that while @MiguelCastillo 's point is valid, we should not get into the habit of stacking feature on top of feature in PRs which makes them not only very large but also increases the time which they take to merge, sometimes are these even never merged as authors get discouraged. So if @swmitra 's PR works and solves the problem it was meant to solve (without introducing new ones), we should merge it and open further suggestions as separate issues.

swmitra · 2017-03-07T07:51:28Z

@ficristo We no longer use that icon in JS code hints. I am a bit confused as I don't see those icons in JS code hints, Can you please confirm once more?

ficristo · 2017-03-07T08:49:12Z

@swmitra you are right. After I had disabled my extensions I don't see it anymore.

swmitra · 2017-03-07T10:48:42Z

Thanks a lot @ficristo for quick confirmation 👍 . Can you please have a look at the changes again as I have tried to address the comments? @petetnt I have wrapped the parameters in a configuration object as per your suggestion, can you please have a look?

… at any font size

swmitra · 2017-03-07T15:20:05Z

@MiguelCastillo Can you please check the implementation now? I just tried to make it extensible by using a less template.

MiguelCastillo · 2017-03-07T15:21:29Z

yup i am checking now. BTW - thank you for clarifying the use case. It definitely helped me understand the scope of what we are accomplishing. I am happy with what we got here :)

MiguelCastillo · 2017-03-07T15:36:31Z

src/view/fontrules/font-based-rules.less

+}
+
+.codehint-menu .dropdown-menu { 
+    max-height: @maxvisibleitems * @lineheight !important; 


how does line height affect this when the fontsize is define in px vs em?

MiguelCastillo · 2017-03-07T15:40:30Z

I like what you have done here! very interesting! I just had a question regarding the max-height value and how it is affected when font size is specified with different units.

swmitra · 2017-03-08T07:46:58Z

@MiguelCastillo I had made core template calculations unit agnostic. Also I had to introduce an additional step to convert only em to px. Rest all units like pt, rem and px works well with this implementation.
Thanks a lot for bringing the point up as it finally resulted in a better implementation 👍.

Please check the code and try out the implementation and let me know if see any issues.

MiguelCastillo · 2017-03-08T21:37:30Z

@swmitra yeah - this is good to merge for me. I am not sure if there are any TODOs pending, are there?

swmitra · 2017-03-09T06:02:33Z

@ficristo Can you please have a look now?

ficristo · 2017-03-09T09:18:33Z

@MiguelCastillo review is more than enough for me.

chylex · 2017-09-25T22:58:25Z

Is there any way to revert this? It feels like I can see way fewer hints than I used to, and any code hints that include documentation look misaligned, so I'd rather go back to the default font size & code hint area height.

Sync code hint font size with code view font size

46ad28c

swmitra requested review from abose, petetnt and ficristo February 10, 2017 08:38

ficristo suggested changes Feb 10, 2017

View reviewed changes

Address Review comments and handle dyanamic width

013c8c5

swmitra added 2 commits March 7, 2017 18:17

Dynamic hint menu height implementations to show only 8 lines of info…

4dc3df0

… at any font size

New implementation based on less template

a481e47

MiguelCastillo reviewed Mar 7, 2017

View reviewed changes

swmitra added 2 commits March 8, 2017 12:34

Handle all units for less templating

43b8d4d

Fix eslint error

def4c85

ficristo approved these changes Mar 9, 2017

View reviewed changes

MiguelCastillo merged commit ad93627 into master Mar 9, 2017

saurabh95 added this to the Release 1.9 milestone Mar 10, 2017

ficristo deleted the swmitra/DyanamicHintFontSize branch March 16, 2017 19:11

swmitra mentioned this pull request Apr 7, 2017

Fix font family name preference #13279

Merged

core-ai-bot mentioned this pull request Aug 30, 2021

[CLOSED] Sync code hint font size with code view font size brackets-archive/bracketsIssues#11113

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync code hint font size with code view font size #13091

Sync code hint font size with code view font size #13091

swmitra commented Feb 10, 2017 •

edited

Loading

ficristo commented Feb 10, 2017

ficristo left a comment

ficristo Feb 10, 2017

ficristo Feb 10, 2017

ficristo Feb 10, 2017

ficristo Feb 10, 2017

petetnt Feb 12, 2017

swmitra commented Feb 13, 2017

MiguelCastillo commented Feb 13, 2017

swmitra commented Mar 7, 2017

zaggino commented Mar 7, 2017

swmitra commented Mar 7, 2017

ficristo commented Mar 7, 2017

swmitra commented Mar 7, 2017

swmitra commented Mar 7, 2017

MiguelCastillo commented Mar 7, 2017

MiguelCastillo Mar 7, 2017

MiguelCastillo commented Mar 7, 2017

swmitra commented Mar 8, 2017

MiguelCastillo commented Mar 8, 2017

swmitra commented Mar 9, 2017

ficristo commented Mar 9, 2017

chylex commented Sep 25, 2017 •

edited

Loading

		@@ -122,10 +128,10 @@ define(function (require, exports, module) {
		* @param {string} value Is the value of the style
		* @param {boolean} important Is a flag to make the style property !important

Sync code hint font size with code view font size #13091

Sync code hint font size with code view font size #13091

Conversation

swmitra commented Feb 10, 2017 • edited Loading

ficristo commented Feb 10, 2017

ficristo left a comment

Choose a reason for hiding this comment

ficristo Feb 10, 2017

Choose a reason for hiding this comment

ficristo Feb 10, 2017

Choose a reason for hiding this comment

ficristo Feb 10, 2017

Choose a reason for hiding this comment

ficristo Feb 10, 2017

Choose a reason for hiding this comment

petetnt Feb 12, 2017

Choose a reason for hiding this comment

swmitra commented Feb 13, 2017

MiguelCastillo commented Feb 13, 2017

swmitra commented Mar 7, 2017

zaggino commented Mar 7, 2017

swmitra commented Mar 7, 2017

ficristo commented Mar 7, 2017

swmitra commented Mar 7, 2017

swmitra commented Mar 7, 2017

MiguelCastillo commented Mar 7, 2017

MiguelCastillo Mar 7, 2017

Choose a reason for hiding this comment

MiguelCastillo commented Mar 7, 2017

swmitra commented Mar 8, 2017

MiguelCastillo commented Mar 8, 2017

swmitra commented Mar 9, 2017

ficristo commented Mar 9, 2017

chylex commented Sep 25, 2017 • edited Loading

swmitra commented Feb 10, 2017 •

edited

Loading

chylex commented Sep 25, 2017 •

edited

Loading