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.

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

Add class to tooltip DOM element distinguish errors from warnings #4810

Merged
merged 2 commits into from
May 31, 2022
Merged

Add class to tooltip DOM element distinguish errors from warnings #4810

merged 2 commits into from
May 31, 2022

Conversation

BenSouchet
Copy link
Contributor

Issue #4799 Styling Tooltip Base on Gutter Annotation Type

Description of changes:

Adding required lines to add a CSS class to the tooltip element to distinguish errors from warnings.

Test by building ace from my branch, it's working like a charm:
Screenshot 2022-05-31 at 20 45 56

Not sure about if it the proper way to reset the className with an hardcoded value tho'

@BenSouchet
Copy link
Contributor Author

The trim() is probably not necessary but I added it because the className value of annotation start with a space, and I haven't seen trim in addCssClass() and/or hasCssClass().

@codecov
Copy link

codecov bot commented May 31, 2022

Codecov Report

Merging #4810 (0617677) into master (57ded09) will increase coverage by 0.04%.
The diff coverage is 16.66%.

@@            Coverage Diff             @@
##           master    #4810      +/-   ##
==========================================
+ Coverage   71.12%   71.16%   +0.04%     
==========================================
  Files         554      554              
  Lines       55711    55716       +5     
  Branches    10419    10420       +1     
==========================================
+ Hits        39623    39651      +28     
+ Misses      16088    16065      -23     
Flag Coverage Δ
unittests 71.16% <16.66%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
lib/ace/mouse/default_gutter_handler.js 25.96% <0.00%> (-0.78%) ⬇️
lib/ace/tooltip.js 43.75% <33.33%> (+0.27%) ⬆️
lib/ace/editor.js 81.63% <0.00%> (+1.90%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 57ded09...0617677. Read the comment docs.

Copy link
Contributor

@andrewnester andrewnester left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Left some comments, but please also add a test for this functionality

@@ -114,6 +114,7 @@ function Tooltip (parentNode) {
this.hide = function() {
if (this.isOpen) {
this.getElement().style.display = "none";
Copy link
Contributor

Choose a reason for hiding this comment

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

You can store this class name in some constant / property so in cass it’s changed we change it in one place

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have created a constant var CLASSNAME

@@ -87,6 +87,7 @@ function GutterHandler(mouseHandler) {
tooltipAnnotation = annotation.text.join("<br/>");

tooltip.setHtml(tooltipAnnotation);
tooltip.setClassName(annotation.className.trim());
Copy link
Contributor

Choose a reason for hiding this comment

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

It might happen that className is undefined or null when annotation type is custom for example, so we should account for this too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added an if statement to ensure we don't call .trim() and setClassName() with an undefined or null value

@BenSouchet
Copy link
Contributor Author

Last point it seems there isn't any test for the tooltip.js file (no tooltip_test.js) is there a way to get or generate a starting point for that test file?

Maybe this merge request isn't the best place for the creation and writing of all the tests of the tooltip.js file.

(I think I can manage to add one test to an existing test file but create the whole file with the setup/init process It's would be too difficult for me 🙁 )

@andrewnester
Copy link
Contributor

@BenSouchet sure, no worries about the test, I really thought we had one for tooltip already

@andrewnester andrewnester merged commit d2446d6 into ajaxorg:master May 31, 2022
@andrewnester
Copy link
Contributor

Merged, thanks for contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants