-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Custom reports: hide "show ..." checkboxes in menu #2174
Merged
Merged
Changes from all commits
Commits
Show all changes
44 commits
Select commit
Hold shift + click to select a range
aed3fdd
Add Toggles
carkom 28975b7
budget table
carkom 1c638f5
testing
carkom bb043eb
updates
carkom 2f52f48
updates
carkom 4b31ab3
fixes
carkom 481a6c9
updates
carkom ea7ae68
Merge remote-tracking branch 'upstream/master' into toggle
carkom 29ba32c
fix Menu
carkom 25ef687
lint fixes
carkom 9d3ae9e
fix keybindings
carkom ae80b8a
revert budget menu changes
carkom 2f0711d
notes
carkom d2b776f
remove default exports
carkom f0aa4a0
fixes
carkom 3aab8bc
disabled fix
carkom 419abd7
add style option
carkom 280a79e
lint fix
carkom 9309435
remove css
carkom d1cff7c
Merge branch 'master' into toggle
carkom de9cc38
lint fixes
carkom f04ddd1
color updates
carkom e266752
Merge branch 'master' into toggle
carkom 46adddf
Merge branch 'master' into toggle
carkom ddebe3f
Merge branch 'master' into toggle
carkom 5cf9ecc
merge menu with togglemenu
carkom 319e18f
Merge branch 'master' into toggle
carkom ce4a0e4
host
carkom 3456b7c
menu fixes
carkom 7cf587e
fix regression
carkom d958c6f
remove host
carkom 65ec3fa
Merge branch 'master' into toggle
carkom fb5ed9f
adjustments
carkom 673732b
onToggle
carkom a1f88eb
Merge branch 'master' into toggle
carkom 17eb080
vrt fix
carkom 13241f9
typecheck
carkom 436d76d
Merge branch 'master' into toggle
carkom cc62af1
Merge branch 'master' into toggle
carkom bd26630
Merge branch 'master' into toggle
carkom f60be9b
merge fixes
carkom 25cbe0c
colors
carkom bd81bba
lint fix
carkom 3c1feca
Merge branch 'master' into toggle
carkom File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,75 @@ | ||
import React from 'react'; | ||
|
||
import { css } from 'glamor'; | ||
|
||
import { theme, type CSSProperties } from '../../style'; | ||
|
||
type ToggleProps = { | ||
id: string; | ||
checked: boolean; | ||
onToggle?: () => void; | ||
onColor?: string; | ||
style?: CSSProperties; | ||
}; | ||
|
||
export const Toggle = ({ | ||
id, | ||
checked, | ||
onToggle, | ||
onColor, | ||
style, | ||
}: ToggleProps) => { | ||
return ( | ||
<div style={{ marginTop: -20, ...style }}> | ||
<input | ||
id={id} | ||
checked={checked} | ||
onChange={onToggle} | ||
className={`${css({ | ||
height: 0, | ||
width: 0, | ||
visibility: 'hidden', | ||
})}`} | ||
type="checkbox" | ||
/> | ||
<label | ||
style={{ | ||
background: checked ? onColor : theme.checkboxToggleBackground, | ||
}} | ||
className={`${css({ | ||
display: 'flex', | ||
alignItems: 'center', | ||
justifyContent: 'space-between', | ||
cursor: 'pointer', | ||
width: '32px', | ||
height: '16px', | ||
borderRadius: '100px', | ||
position: 'relative', | ||
transition: 'background-color .2s', | ||
})}`} | ||
htmlFor={id} | ||
> | ||
<span | ||
className={`${css( | ||
{ | ||
content: '', | ||
position: 'absolute', | ||
top: '2px', | ||
left: '2px', | ||
width: '12px', | ||
height: '12px', | ||
borderRadius: '100px', | ||
transition: '0.2s', | ||
background: '#fff', | ||
boxShadow: '0 0 2px 0 rgba(10, 10, 10, 0.29)', | ||
}, | ||
checked && { | ||
left: 'calc(100% - 2px)', | ||
transform: 'translateX(-100%)', | ||
}, | ||
)}`} | ||
/> | ||
</label> | ||
</div> | ||
); | ||
}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
--- | ||
category: Enhancements | ||
authors: [carkom] | ||
--- | ||
|
||
Hide "show ..." checkboxes within menu for custom reports page. Introduce toggle switches. |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Would it work if we introduce a new
TogglableMenuItem
which extends fromMenuItem
?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 only works if we are looking to group toggled items and non-toggled items. If you want a menu with (regular item, toggle item, regular item, toggle item) then a separate toggleitem group would not work without an overly complicated code set. I know we have a couple complex menus right now. Not sure there's any that fit this use case currently but I can imagine we may want this ability in the future.
Thoughts?
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.
@joel-jeremy I'd love to have your thoughts on grouping toggle items at top or bottom vs my proposal which would allow for them to be used anywhere in the list. Cheers!
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 think it wouldn't hurt to allow the consumer of the component to place the toggle/regular item wherever it sees fit. It could group the toggles together or not, the component provides the flexibility. :)
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.
Hey @joel-jeremy, I've sent you a couple messages via discord. I don't see a way to implement how you are suggesting. Can you give me some guidance? If you don't have time to write anything right now, would you be okay with approving as is and we can adjust it to your vision in a new PR?