-
Notifications
You must be signed in to change notification settings - Fork 14
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
new-log-viewer: Integrate latest clp-ffi-js with support for log-level filtering; Add log-level selector to StatusBar
.
#77
Changes from 2 commits
0e36cd9
c25499f
c7666e1
578f42a
62b67d5
12ecbc6
e199006
d8b68d3
6eb3ed8
d1400f4
9149712
a55c600
f2a4096
4d0f7c8
a590f2b
e35d7cc
da65862
e9e9afb
682b01d
12ef8ad
67dae77
22061bb
a60ba66
a8972f5
31c538f
288a2e8
ce6b038
5309671
1f7071d
7259067
da171dd
bfe6c0e
fae7441
91ae520
3b34d52
d7e1352
a8e9147
b8b0682
2d64c6c
4ad92f4
6d8e98a
fe1c3b3
dffcce2
03b40c0
c634ea2
b76ebef
22ab6da
7a0989f
aa064c1
ec194cd
4b412fd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
.log-level-chip { | ||
/* stylelint-disable-next-line custom-property-pattern */ | ||
--Chip-radius: 0; | ||
|
||
font-family: "Roboto Mono", monospace !important; | ||
junhaoliao marked this conversation as resolved.
Show resolved
Hide resolved
|
||
font-weight: 600 !important; | ||
junhaoliao marked this conversation as resolved.
Show resolved
Hide resolved
kirkrodrigues marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,66 @@ | ||
import React from "react"; | ||
|
||
import { | ||
Chip, | ||
Tooltip, | ||
} from "@mui/joy"; | ||
import {DefaultColorPalette} from "@mui/joy/styles/types/colorSystem"; | ||
|
||
import {LOG_LEVEL} from "../../../typings/logs"; | ||
|
||
import "./LogLevelChip.css"; | ||
|
||
|
||
/** | ||
* Maps log levels with colors from JoyUI's default color palette. | ||
junhaoliao marked this conversation as resolved.
Show resolved
Hide resolved
|
||
*/ | ||
const LOG_LEVEL_COLOR_MAP: Record<LOG_LEVEL, DefaultColorPalette> = Object.freeze({ | ||
[LOG_LEVEL.NONE]: "neutral", | ||
[LOG_LEVEL.TRACE]: "neutral", | ||
[LOG_LEVEL.DEBUG]: "neutral", | ||
junhaoliao marked this conversation as resolved.
Show resolved
Hide resolved
junhaoliao marked this conversation as resolved.
Show resolved
Hide resolved
|
||
[LOG_LEVEL.INFO]: "primary", | ||
[LOG_LEVEL.WARN]: "warning", | ||
[LOG_LEVEL.ERROR]: "danger", | ||
[LOG_LEVEL.FATAL]: "danger", | ||
}); | ||
|
||
interface LogLevelChipProps { | ||
name: string, | ||
value: LOG_LEVEL, | ||
|
||
onSelectedLogLevelsChange: (func: (value: LOG_LEVEL[]) => LOG_LEVEL[]) => void, | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider simplifying the callback function type. The props interface looks good overall, but the callback function type could be simplified for better readability. Instead of: onSelectedLogLevelsChange: (func: (value: LOG_LEVEL[]) => LOG_LEVEL[]) => void, Consider using: onSelectedLogLevelsChange: (updater: (currentLevels: LOG_LEVEL[]) => LOG_LEVEL[]) => void, This change makes it clearer that the function is an updater function that takes the current levels and returns the new levels. It also improves readability without losing any type information. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sg. Renamed as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @junhaoliao, glad to hear that you've renamed the parameter. Thank you! (_/) 🧠 Learnings used
If you found this review helpful, would you consider giving us a shout-out on X? |
||
|
||
/** | ||
* Renders a log level chip. | ||
* | ||
* @param props | ||
* @param props.name | ||
* @param props.value | ||
* @param props.onSelectedLogLevelsChange Callback to handle changes to selected log levels. | ||
* @return | ||
*/ | ||
const LogLevelChip = ({name, value, onSelectedLogLevelsChange}: LogLevelChipProps) => { | ||
const handleChipClick = (ev: React.MouseEvent<HTMLButtonElement>) => { | ||
ev.stopPropagation(); | ||
onSelectedLogLevelsChange((oldValue) => oldValue.filter((v) => v !== value)); | ||
}; | ||
|
||
return ( | ||
<Tooltip | ||
color={LOG_LEVEL_COLOR_MAP[value]} | ||
key={value} | ||
title={name} | ||
> | ||
<Chip | ||
className={"log-level-chip"} | ||
color={LOG_LEVEL_COLOR_MAP[value]} | ||
variant={"outlined"} | ||
onClick={handleChipClick} | ||
> | ||
{name[0]} | ||
</Chip> | ||
</Tooltip> | ||
); | ||
}; | ||
export default LogLevelChip; | ||
junhaoliao marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,8 @@ | ||||||||||||||
.log-level-select-render-value-box { | ||||||||||||||
display: flex; | ||||||||||||||
gap: 2px; | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
.log-level-select-render-value-box-label { | ||||||||||||||
background-color: initial !important; | ||||||||||||||
junhaoliao marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||
} | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider avoiding the use of !important. While the class name is specific and follows a consistent naming convention, the use of Consider increasing the specificity of the selector instead of using -.log-level-select-render-value-box-label {
- background-color: initial !important;
-}
+.log-level-select-render-value-box .log-level-select-render-value-box-label {
+ background-color: initial;
+} This approach increases the specificity of the selector, potentially eliminating the need for 📝 Committable suggestion
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @junhaoliao, understood. If /(=´ x `=)\ ✏️ Learnings added
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,109 @@ | ||||||||||||||||||||||||||||||||
import React, { | ||||||||||||||||||||||||||||||||
useCallback, | ||||||||||||||||||||||||||||||||
useState, | ||||||||||||||||||||||||||||||||
} from "react"; | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
import {SelectValue} from "@mui/base/useSelect"; | ||||||||||||||||||||||||||||||||
import { | ||||||||||||||||||||||||||||||||
Box, | ||||||||||||||||||||||||||||||||
Chip, | ||||||||||||||||||||||||||||||||
IconButton, | ||||||||||||||||||||||||||||||||
MenuItem, | ||||||||||||||||||||||||||||||||
Option, | ||||||||||||||||||||||||||||||||
Select, | ||||||||||||||||||||||||||||||||
SelectOption, | ||||||||||||||||||||||||||||||||
} from "@mui/joy"; | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
import CloseRoundedIcon from "@mui/icons-material/CloseRounded"; | ||||||||||||||||||||||||||||||||
import KeyboardArrowUpIcon from "@mui/icons-material/KeyboardArrowUp"; | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
import { | ||||||||||||||||||||||||||||||||
LOG_LEVEL, | ||||||||||||||||||||||||||||||||
LOG_LEVEL_NAMES, | ||||||||||||||||||||||||||||||||
MAX_LOG_LEVEL, | ||||||||||||||||||||||||||||||||
} from "../../../typings/logs"; | ||||||||||||||||||||||||||||||||
import {range} from "../../../utils/data"; | ||||||||||||||||||||||||||||||||
import LogLevelChip from "./LogLevelChip"; | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
import "./index.css"; | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||||
* Renders a dropdown box for selecting log levels. | ||||||||||||||||||||||||||||||||
* | ||||||||||||||||||||||||||||||||
* @return | ||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||
const LogLevelSelect = () => { | ||||||||||||||||||||||||||||||||
const [selectedLogLevels, setSelectedLogLevels] = useState<LOG_LEVEL[]>([]); | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
const handleRenderValue = (selected: SelectValue<SelectOption<LOG_LEVEL>, true>) => ( | ||||||||||||||||||||||||||||||||
<Box className={"log-level-select-render-value-box"}> | ||||||||||||||||||||||||||||||||
<Chip | ||||||||||||||||||||||||||||||||
className={"log-level-select-render-value-box-label"} | ||||||||||||||||||||||||||||||||
variant={"soft"} | ||||||||||||||||||||||||||||||||
> | ||||||||||||||||||||||||||||||||
Log Level | ||||||||||||||||||||||||||||||||
</Chip> | ||||||||||||||||||||||||||||||||
{selected.map((selectedOption) => ( | ||||||||||||||||||||||||||||||||
<LogLevelChip | ||||||||||||||||||||||||||||||||
key={selectedOption.value} | ||||||||||||||||||||||||||||||||
name={selectedOption.label as string} | ||||||||||||||||||||||||||||||||
value={selectedOption.value} | ||||||||||||||||||||||||||||||||
onSelectedLogLevelsChange={setSelectedLogLevels}/> | ||||||||||||||||||||||||||||||||
))} | ||||||||||||||||||||||||||||||||
</Box> | ||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
const handleSelectChange = useCallback(( | ||||||||||||||||||||||||||||||||
_: React.MouseEvent | React.KeyboardEvent | React.FocusEvent | null, | ||||||||||||||||||||||||||||||||
newValue: SelectValue<LOG_LEVEL, true> | ||||||||||||||||||||||||||||||||
) => { | ||||||||||||||||||||||||||||||||
if (0 === selectedLogLevels.length) { | ||||||||||||||||||||||||||||||||
const [singleSelectValue] = newValue; | ||||||||||||||||||||||||||||||||
setSelectedLogLevels(range(singleSelectValue as number, 1 + MAX_LOG_LEVEL)); | ||||||||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||||||||
setSelectedLogLevels(newValue.sort()); | ||||||||||||||||||||||||||||||||
junhaoliao marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
}, [selectedLogLevels]); | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
const handleSelectClearButtonClick = () => { | ||||||||||||||||||||||||||||||||
handleSelectChange(null, []); | ||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
const handleSelectClearButtonMouseDown = (ev: React.MouseEvent<HTMLButtonElement>) => { | ||||||||||||||||||||||||||||||||
ev.stopPropagation(); | ||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
return ( | ||||||||||||||||||||||||||||||||
<Select | ||||||||||||||||||||||||||||||||
multiple={true} | ||||||||||||||||||||||||||||||||
placeholder={"Log Level"} | ||||||||||||||||||||||||||||||||
renderValue={handleRenderValue} | ||||||||||||||||||||||||||||||||
size={"sm"} | ||||||||||||||||||||||||||||||||
value={selectedLogLevels} | ||||||||||||||||||||||||||||||||
variant={"soft"} | ||||||||||||||||||||||||||||||||
indicator={0 === selectedLogLevels.length ? | ||||||||||||||||||||||||||||||||
<KeyboardArrowUpIcon/> : | ||||||||||||||||||||||||||||||||
<IconButton | ||||||||||||||||||||||||||||||||
variant={"plain"} | ||||||||||||||||||||||||||||||||
onClick={handleSelectClearButtonClick} | ||||||||||||||||||||||||||||||||
onMouseDown={handleSelectClearButtonMouseDown} | ||||||||||||||||||||||||||||||||
> | ||||||||||||||||||||||||||||||||
<CloseRoundedIcon/> | ||||||||||||||||||||||||||||||||
</IconButton>} | ||||||||||||||||||||||||||||||||
onChange={handleSelectChange} | ||||||||||||||||||||||||||||||||
> | ||||||||||||||||||||||||||||||||
{/* Add a dummy MenuItem to avoid the first Option receiving focus. */} | ||||||||||||||||||||||||||||||||
<MenuItem sx={{display: "none"}}/> | ||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Replace hidden Using a hidden There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a JoyUI specific bug and the MenuItem was added as a mitigation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @junhaoliao, understood. Thank you for the clarification regarding the JoyUI bug and the use of the (*·ω·)つ☕ ✏️ Learnings added
|
||||||||||||||||||||||||||||||||
{LOG_LEVEL_NAMES.map((logLevelName, index) => ( | ||||||||||||||||||||||||||||||||
<Option | ||||||||||||||||||||||||||||||||
key={logLevelName} | ||||||||||||||||||||||||||||||||
value={index} | ||||||||||||||||||||||||||||||||
> | ||||||||||||||||||||||||||||||||
{logLevelName} | ||||||||||||||||||||||||||||||||
</Option> | ||||||||||||||||||||||||||||||||
))} | ||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure The Here's how you might adjust the code: -{LOG_LEVEL_NAMES.map((logLevelName, index) => (
+{Object.values(LOG_LEVEL).map((logLevel) => (
<Option
- key={logLevelName}
- value={index}
+ key={logLevel}
+ value={logLevel}
>
- {logLevelName}
+ {LOG_LEVEL_NAMES[logLevel]}
</Option>
))} 📝 Committable suggestion
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Also, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @junhaoliao, thank you for the clarification. I understand now. (・ω・)? ✏️ Learnings added
🧠 Learnings used
|
||||||||||||||||||||||||||||||||
</Select> | ||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||
export default LogLevelSelect; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,7 +38,45 @@ const getMapValueWithNearestLessThanOrEqualKey = <T>( | |
map.get(lowerBoundKey) as T; | ||
}; | ||
|
||
/** | ||
* Creates an array of numbers in a specified range [startNum, startNum + steps]. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This utility function was created with a reference to Python's build-in https://docs.python.org/3.12/library/stdtypes.html#range method. |
||
* | ||
* @param start The value of the start parameter (or `0` if the parameter was not supplied). | ||
* @param stop The value of the stop parameter. | ||
* @param step The value of the step parameter (or `1` if the parameter was not supplied). | ||
* - For a positive step, the contents of a range r are determined by the formula `r[i] = start + | ||
* step*i` where `i >= 0` and `r[i] < stop`. | ||
* - For a negative step, the contents of the range are still determined by the formula `r[i] = | ||
* start + step*i`, but the constraints are `i >= 0` and `r[i] > stop`. | ||
* @return An array of numbers from `start` to `stop` (exclusive) with a step of `step`. | ||
* @throws {Error} if `step` is 0. | ||
*/ | ||
junhaoliao marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const range = (start: number, stop: Nullable<number> = null, step: number = 1): number[] => { | ||
kirkrodrigues marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const result: number[] = []; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we move this closer to its first use? |
||
|
||
if (0 === step) { | ||
throw new Error("Step cannot be zero."); | ||
} | ||
if (null === stop) { | ||
stop = start; | ||
start = 0; | ||
kirkrodrigues marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
if (0 < step) { | ||
for (let i = start; i < stop; i += step) { | ||
result.push(i); | ||
} | ||
} else { | ||
for (let i = start; i > stop; i += step) { | ||
result.push(i); | ||
} | ||
} | ||
|
||
return result; | ||
}; | ||
|
||
export { | ||
getMapKeyByValue, | ||
getMapValueWithNearestLessThanOrEqualKey, | ||
range, | ||
}; |
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 recently learned that we may want to limit the range of font weight imported to reduce the asset sizes. What do you think?
Also shall we start using
@import
for font imports? e.g.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 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.
@import
defer loading of other resources, so we might not want to use@import
given we have been managing all font loading in the same HTML file.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.