fix(site): disable auto fields when they are disabled in the template settings #10022
Conversation
…o-stop-showing-when-not
There was a problem hiding this comment.
Looks good! One thing I noticed is that if you already had auto-start/stop set up and then they are disabled via the template, you can still change the time/day/hour fields and you can still hit Submit but nothing actually updates. Should we disable those fields as well?
| & > svg { | ||
| return ( | ||
| <div | ||
| ref={ref} |
There was a problem hiding this comment.
I am probably missing something obvious, but what does passing in ref here do?
There was a problem hiding this comment.
The ref gives any parent component the ability to manipulate the the div element created by Pill.
So for example:
function Parent () {
const divRef = useRef<HTMLDivElement>(null);
return <Pill ref={divRef} {...otherProps} />
}With this setup, the parent would be able to manipulate the real HTML div element that Pill creates from inside useEffect and event handlers, even though the div exists in a separate component
There was a problem hiding this comment.
But judging from this PR, I think it's a preemptive change to make the component more reusable. I don't see any of the <Pill /> instances in the rest of the code using it
There was a problem hiding this comment.
Yes, it is used by the Tooltip component that wraps the Pill
There was a problem hiding this comment.
Ohhhhh I see. I see it in the docs. Unfortunate that it is not reflected in the code! I guess you just have to know how MUI tooltip works. Thanks for the explanations!
|
Whoops lol I misread my GitHub notifications and just noticed this was assigned to someone else, I will remove the assignment since I already reviewed haha. Sorry all. |
| } | ||
| return themeStyles(type, lightBorder); | ||
| }, [type, lightBorder]); | ||
| const typeStyles = useMemo(() => { |
There was a problem hiding this comment.
Does this need to be memoized? I guess it just feels like the logic wouldn't have much overhead, and it's only being used within the same component
There was a problem hiding this comment.
I don't think so, I guess it is only using memo as a way to wrap/encapsulate it into a function but I'm just guessing.
| & > svg { | ||
| return ( | ||
| <div | ||
| ref={ref} |
There was a problem hiding this comment.
The ref gives any parent component the ability to manipulate the the div element created by Pill.
So for example:
function Parent () {
const divRef = useRef<HTMLDivElement>(null);
return <Pill ref={divRef} {...otherProps} />
}With this setup, the parent would be able to manipulate the real HTML div element that Pill creates from inside useEffect and event handlers, even though the div exists in a separate component
| & > svg { | ||
| return ( | ||
| <div | ||
| ref={ref} |
There was a problem hiding this comment.
But judging from this PR, I think it's a preemptive change to make the component more reusable. I don't see any of the <Pill /> instances in the rest of the code using it
|
Just reposting this because I did not see a reply to it: One thing I noticed is that if you already had auto-start/stop set up and then they are disabled via the template, you can still change the time/day/hour fields and you can still hit Submit but nothing actually updates. Should we disable those fields as well? |
|
@code-asher I missed this comment, sorry! I think what you suggest is the right move. I will fix it. |
Fix #9820