-
-
Notifications
You must be signed in to change notification settings - Fork 250
Show the number of errors and warnings in the Problems
tab's title
#1004
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
base: master
Are you sure you want to change the base?
Conversation
@mediremi is attempting to deploy a commit to the ReScript Association Team on Vercel. A member of the Team first needs to authorize it. |
@@ -1666,16 +1716,14 @@ let make = (~versions: array<string>) => { | |||
| Problems => "Problems" | |||
| Settings => "Settings" | |||
} | |||
let onMouseDown = evt => { | |||
let onClick = evt => { |
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.
Using onClick
instead of onMouseDown
makes the tabs keyboard accessible - buttons with a click handler can be triggered with Enter
/Space
.
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.
This might have been an optimisation so that event.preventDefault()
gets invoked faster. Or maybe it was deliberate that it does not get focused.
@ryyppy do you still remember?
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 most likely a good reason. I think it had something to do with touch event delay. Ultimately one should consider adapting a accessibility component lib like react-aria to have a fully accessible tab bar etc.
Problems
tab's title
src/Playground.res
Outdated
let onWarningFlagsResetClick = _evt => { | ||
setConfig({ | ||
...config, | ||
warn_flags: "+a-4-9-20-41-50-61-102-109", |
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've removed warnings 40
and 42
from warn_flags
as they no longer exist in the compiler: https://github.com/rescript-lang/rescript/blob/9a8f133f6f1bbc84975efceb8113b767805fbcfd/compiler/ext/warnings.mli#L57-L59
I've made a separate PR to the compiler repo for updating Bsc_warnings.defaults_w
, which also still includes these warnings.
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've also removed warnings:
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.
We support older compiler versions here, which might still support those warnings?
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.
We should probably have modern (V12-level) warning defaults, just when v12 is selected, and keep this one for the older compilers.
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.
The default warnings come from the compiler (specifically, with RescriptCompilerApi.Compiler.getConfig
) - this string just determines the warnings used when the user clicks on [reset]
in the 'Warning Flags' section.
Ideally, we'd want to reset the warnings to the initial defaults the compiler returned, but I couldn't figure that out. So I've restored the value of this string back to what it was before ("+a-4-9-20-40-41-42-50-61-102-109"
)
let onResetClick = evt => { | ||
ReactEvent.Mouse.preventDefault(evt) | ||
|
||
let open_modules = switch readyState.selected.apiVersion { |
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.
This is gone? We still need this (for v11 at least).
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 renamed this callback to onWarningFlagsResetClick
to make it clearer that this is triggered by clicking on [reset]
in the 'Warnings flags' section of the settings panel.
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 removed the open_modules
update part of this function as:
i) there's no way for users to configure open modules (at least as far as I can tell)
ii) the updating of open_modules
on compiler load and switch is actually managed by the useEffect
in CompilerManagerHook
- this was just duplicated logic from CompilerManagerHook.getOpenModules
Great stuff. Also great that you took on a little refactor here, but I am afraid we need to support older versions as well so if you adapt to something that only exists in v12, please guard it behind a version check. |
I've moved the warning & error counts to after the title 👍
Now that I've restored the warning flags reset value, I think everything should still work on older versions exactly as it did before. I've had a quick check with versions 8 through 12, and everything seems to work fine. Let me know if I've broken anything and I'll be happy to add version checks |
Problems
tab's titleInt.toString
instead ofBelt.Int.toString
if the ReScript major version is 11+React
andConsole
panels in theOutput
tab