You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@devlake.apache.org by GitBox <gi...@apache.org> on 2022/09/17 05:41:15 UTC
[GitHub] [incubator-devlake] henit-chobisa opened a new pull request, #3107: feat: Responsive sidebar
henit-chobisa opened a new pull request, #3107:
URL: https://github.com/apache/incubator-devlake/pull/3107
# Summary
## Additions :
- Added `UIContext` for Universal State Management of the Sidebar.
- Added Conditional Rendering to the Sidebar.
- Added new `Navbar Item`, `menu` button, that toggles the sidebar on smaller window sizes.
## Effect :
- UI Context Added for further UI State Management.
- Responsive Sidebar would be implemented in the Config-UI.
- There would be a toggle button for seen in the smaller screen sizes.
### Does this close any open issues?
Closes #3106
### Screenshots
### Other Information
Any other information that is important to this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] henit-chobisa commented on pull request #3107: feat: Responsive sidebar
Posted by GitBox <gi...@apache.org>.
henit-chobisa commented on PR #3107:
URL: https://github.com/apache/incubator-devlake/pull/3107#issuecomment-1254011129
>
@e2corporation
Sure, Julien!!
I was also a bit scared of wrapping the whole app with the `UIContext` 😅 but I couldn't find any alternative way
If I would have wrapped it with the page, still the amount of renders wouldn't have reduces.
What an amazing idea!!, Julien, this is why I love open source, Creativity. 😍
Docks are trendy these day, we can also give our application that modern look, and I also look forward implementing something in the dock which gives it that unique look which people might copy! ⚡️
I would love to work on this 🫡
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] e2corporation commented on a diff in pull request #3107: feat: Responsive sidebar
Posted by GitBox <gi...@apache.org>.
e2corporation commented on code in PR #3107:
URL: https://github.com/apache/incubator-devlake/pull/3107#discussion_r975648117
##########
config-ui/src/components/Nav.jsx:
##########
@@ -26,11 +26,41 @@ import {
import '@/styles/nav.scss'
import { ReactComponent as SlackIcon } from '@/images/slack-mark-monochrome-black.svg'
import { ReactComponent as SlackLogo } from '@/images/slack-rgb.svg'
+import UIContext from '../store/UIContext'
+import useWindowSize from '../hooks/useWIndowSize'
+
const Nav = () => {
+ const uiContext = useContext(UIContext);
+ const [menuClass, setMenuClass] = useState("navbarMenuButton");
+ const size = useWindowSize();
+
+ const toggleSidebarOpen = (open) => {
Review Comment:
I think this could be refactored to be more declarative and drop the need for using the `IF` and `return` statements.
```suggestion
const toggleSidebarOpen = (open) => {
uiContext.changeSidebarVisibility(open)
setMenuClass(open ? 'navbarMenuButtonSidebarOpened' : 'navbarMenuButton'
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] e2corporation commented on a diff in pull request #3107: feat: Responsive sidebar
Posted by GitBox <gi...@apache.org>.
e2corporation commented on code in PR #3107:
URL: https://github.com/apache/incubator-devlake/pull/3107#discussion_r975648117
##########
config-ui/src/components/Nav.jsx:
##########
@@ -26,11 +26,41 @@ import {
import '@/styles/nav.scss'
import { ReactComponent as SlackIcon } from '@/images/slack-mark-monochrome-black.svg'
import { ReactComponent as SlackLogo } from '@/images/slack-rgb.svg'
+import UIContext from '../store/UIContext'
+import useWindowSize from '../hooks/useWIndowSize'
+
const Nav = () => {
+ const uiContext = useContext(UIContext);
+ const [menuClass, setMenuClass] = useState("navbarMenuButton");
+ const size = useWindowSize();
+
+ const toggleSidebarOpen = (open) => {
Review Comment:
I think this could be refactored to be more declarative and drop the need for using the `IF` and `return` statements.
```suggestion
const toggleSidebarOpen = (open) => {
uiContext.changeSidebarVisibility(open)
setMenuClass(open ? 'navbarMenuButtonSidebarOpened' : 'navbarMenuButton'
}
```
If the initial state is always guaranteed to be `navbarMenuButton` as the fallback, you could also use the callback mode of the state setter and rewrite the above statement like this.
```suggestion
const toggleSidebarOpen = (open) => {
uiContext.changeSidebarVisibility(open)
setMenuClass((currentMenuClass) => open ? 'navbarMenuButtonSidebarOpened' : currentMenuClass
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] henit-chobisa commented on pull request #3107: feat: Responsive sidebar
Posted by GitBox <gi...@apache.org>.
henit-chobisa commented on PR #3107:
URL: https://github.com/apache/incubator-devlake/pull/3107#issuecomment-1258438014
> @henit-chobisa Additionally, the commit message structure for the last 2 commits does not conform to the current linter format (`fix:` `feat:` `refactor:` `chore:` etc) and is also causing the **Commit Message** CI Check to fail, can those commits messages be amended as well to fit the desired format?
>
> <img alt="Screen Shot 2022-09-26 at 2 16 26 PM" width="577" src="https://user-images.githubusercontent.com/1742233/192351052-09c46ece-ec84-42f9-a70a-b48656493980.png">
>
> **Expected format to pass CI Check**
>
> * chore: removed makefile
> * chore: removed blueprint.go
>
> **or**
>
> * refactor: removed makefile
> * refactor: removed blueprint.go
I'm extremely sorry, I just forgot that
I fix that now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] e2corporation commented on a diff in pull request #3107: feat: Responsive sidebar
Posted by GitBox <gi...@apache.org>.
e2corporation commented on code in PR #3107:
URL: https://github.com/apache/incubator-devlake/pull/3107#discussion_r977926583
##########
config-ui/src/components/Nav.jsx:
##########
@@ -26,11 +26,30 @@ import {
import '@/styles/nav.scss'
import { ReactComponent as SlackIcon } from '@/images/slack-mark-monochrome-black.svg'
import { ReactComponent as SlackLogo } from '@/images/slack-rgb.svg'
+import UIContext from '../store/UIContext'
+import useWindowSize from '../hooks/useWIndowSize'
Review Comment:
```suggestion
import useWindowSize from '@/hooks/useWIndowSize'
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] e2corporation commented on a diff in pull request #3107: feat: Responsive sidebar
Posted by GitBox <gi...@apache.org>.
e2corporation commented on code in PR #3107:
URL: https://github.com/apache/incubator-devlake/pull/3107#discussion_r975657116
##########
config-ui/src/styles/nav.scss:
##########
@@ -17,11 +17,43 @@
position: fixed;
top: 0;
border-radius: 0;
- background: #ffffff;
box-shadow: none;
z-index: 1;
}
+.navbarMenuButton{
+ position: absolute;
+ display: none;
+ color: #7497F7;
+ &:hover {
+ color: #E8471C;
+ }
+
+ @media only screen and (max-width: 850px) {
+ display: flex;
+ left: 0px;
+ }
+}
+
+.navbarMenuButtonSidebarOpened{
+ position: absolute;
+ display: none;
+ color: #7497F7;
+ &:hover {
+ color: #E8471C;
+ }
+
+ @media only screen and (max-width: 850px) {
+ display: flex;
+ left: 240px;
+ }
+}
+
+.navbarItems{
Review Comment:
This css rule is missing a space before the opening bracket `{`
```suggestion
.navbarItems {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] e2corporation commented on pull request #3107: feat: Responsive sidebar
Posted by GitBox <gi...@apache.org>.
e2corporation commented on PR #3107:
URL: https://github.com/apache/incubator-devlake/pull/3107#issuecomment-1258423914
@henit-chobisa Thanks Henit, can you also resolve the ESLint errors mentioned on the **Frontend-Lint** Github CI Check? There are some missing effect dependencies. https://github.com/apache/incubator-devlake/actions/runs/3129884127/jobs/5079735842
In regards to the error of `bind` usage, Instead of using bind, wrap the handler in a closure so there won't be a need for using bind `() => {}`
<img width="1107" alt="Screen Shot 2022-09-26 at 2 09 33 PM" src="https://user-images.githubusercontent.com/1742233/192349518-0860096c-aeb9-42be-a93b-13374838945d.png">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] henit-chobisa closed pull request #3107: feat: Responsive sidebar
Posted by GitBox <gi...@apache.org>.
henit-chobisa closed pull request #3107: feat: Responsive sidebar
URL: https://github.com/apache/incubator-devlake/pull/3107
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] e2corporation commented on a diff in pull request #3107: feat: Responsive sidebar
Posted by GitBox <gi...@apache.org>.
e2corporation commented on code in PR #3107:
URL: https://github.com/apache/incubator-devlake/pull/3107#discussion_r975649772
##########
config-ui/src/components/Nav.jsx:
##########
@@ -26,11 +26,41 @@ import {
import '@/styles/nav.scss'
import { ReactComponent as SlackIcon } from '@/images/slack-mark-monochrome-black.svg'
import { ReactComponent as SlackLogo } from '@/images/slack-rgb.svg'
+import UIContext from '../store/UIContext'
+import useWindowSize from '../hooks/useWIndowSize'
+
const Nav = () => {
+ const uiContext = useContext(UIContext);
+ const [menuClass, setMenuClass] = useState("navbarMenuButton");
+ const size = useWindowSize();
+
+ const toggleSidebarOpen = (open) => {
+ uiContext.changeSidebarVisibility(open)
+ if (open){
+ setMenuClass("navbarMenuButtonSidebarOpened");
+ return;
+ }
+ setMenuClass("navbarMenuButton");
+ }
+
+ useEffect(() => {
Review Comment:
This Effect could also be refactored to be more declarative and remove the nested `IF` statements
```suggestion
useEffect(() => {
toggleSidebarOpen((size.width >= 850 && uiContext.sidebarVisible != true) ? true : false)
}, [size]);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] e2corporation commented on a diff in pull request #3107: feat: Responsive sidebar
Posted by GitBox <gi...@apache.org>.
e2corporation commented on code in PR #3107:
URL: https://github.com/apache/incubator-devlake/pull/3107#discussion_r975656763
##########
config-ui/src/styles/nav.scss:
##########
@@ -17,11 +17,43 @@
position: fixed;
top: 0;
border-radius: 0;
- background: #ffffff;
box-shadow: none;
z-index: 1;
}
+.navbarMenuButton{
Review Comment:
This css rule is missing a space before the opening bracket `{`
```suggestion
.navbarMenuButton {
```
##########
config-ui/src/styles/nav.scss:
##########
@@ -17,11 +17,43 @@
position: fixed;
top: 0;
border-radius: 0;
- background: #ffffff;
box-shadow: none;
z-index: 1;
}
+.navbarMenuButton{
+ position: absolute;
+ display: none;
+ color: #7497F7;
+ &:hover {
+ color: #E8471C;
+ }
+
+ @media only screen and (max-width: 850px) {
+ display: flex;
+ left: 0px;
+ }
+}
+
+.navbarMenuButtonSidebarOpened{
Review Comment:
This css rule is missing a space before the opening bracket `{`
```suggestion
.navbarMenuButtonSidebarOpened {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] e2corporation commented on a diff in pull request #3107: feat: Responsive sidebar
Posted by GitBox <gi...@apache.org>.
e2corporation commented on code in PR #3107:
URL: https://github.com/apache/incubator-devlake/pull/3107#discussion_r977926370
##########
config-ui/src/components/Nav.jsx:
##########
@@ -26,11 +26,30 @@ import {
import '@/styles/nav.scss'
import { ReactComponent as SlackIcon } from '@/images/slack-mark-monochrome-black.svg'
import { ReactComponent as SlackLogo } from '@/images/slack-rgb.svg'
+import UIContext from '../store/UIContext'
Review Comment:
We can use `@` import aliases here for consistency and prevent need for parent dir `..` levels,
```suggestion
import UIContext from '@/store/UIContext'
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] e2corporation commented on pull request #3107: feat: Responsive sidebar
Posted by GitBox <gi...@apache.org>.
e2corporation commented on PR #3107:
URL: https://github.com/apache/incubator-devlake/pull/3107#issuecomment-1252726828
> Thank You @e2corporation for such a detailed Review, I will cover up on all your points and push the fixtures by tomorrow EOD
Great work so far Henit! I'd like to run some functional tests later as well since I had some questions with the resize handling. I see the cleanup function has been added to the resize effect which is nice and will prevent memory leaks. One caveat of wrapping the whole app in a Context too is that it may cause unwanted re-renders which could impact form control components, so I just wanted to make sure there aren't any potential side effects.
I also had another idea to extend this feature, which is to create a "Docked" mode for the sidebar (slim bar with DevLake logo + icons), which maybe something you might be interested in implementing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] e2corporation commented on pull request #3107: feat: Responsive sidebar
Posted by GitBox <gi...@apache.org>.
e2corporation commented on PR #3107:
URL: https://github.com/apache/incubator-devlake/pull/3107#issuecomment-1255362784
> Thank You @e2corporation for such a detailed Review, I will cover up on all your points and push the fixtures by tomorrow EOD
Thanks for applying the updates Henit, we also did much refactoring on `main` branch recently to apply code formatting standards to the UI (`@config-ui`). This PR will need to be rebased, or to prevent conflicts you can re-create a new branch from `main` and cherry-pick all these changes to that new branch and re-force push.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] henit-chobisa commented on pull request #3107: feat: Responsive sidebar
Posted by GitBox <gi...@apache.org>.
henit-chobisa commented on PR #3107:
URL: https://github.com/apache/incubator-devlake/pull/3107#issuecomment-1258400695
> @henit-chobisa Thanks for the updates Henit, it does look like we have some unwanted **backend** files on this PR that will need to be removed. Can these resources be reset to `HEAD` version from `main` so they are discarded from this PR?
>
> * `Makefile`
> * `plugins/`
Julien, I have made the changes you asked for 🫡
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] e2corporation commented on pull request #3107: feat: Responsive sidebar
Posted by GitBox <gi...@apache.org>.
e2corporation commented on PR #3107:
URL: https://github.com/apache/incubator-devlake/pull/3107#issuecomment-1259953885
> > @henit-chobisa Another issue that may affect this PR from a clean merge is that you have issued `merge` commits when updating changes from `main` branch, when we prefer a `rebase` policy instead (ie `git pull --rebase`) that way feature commits are replayed on top of main cleanly.
>
> So should I undo the merge commits and do rebase with that ?
It still looks like it would be a manual cherry-pick process to be able to resolve the issue. I would create a local clone/backup of this branch. Create a new branch from `main` as the starting point, cherry-pick all relevant changes to working copy and then force-push the branch to `henit-chobisa:Responsive-Sidebar`. This PR can also be closed and we can re-issue a new PR from another branch as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] e2corporation commented on a diff in pull request #3107: feat: Responsive sidebar
Posted by GitBox <gi...@apache.org>.
e2corporation commented on code in PR #3107:
URL: https://github.com/apache/incubator-devlake/pull/3107#discussion_r980359346
##########
config-ui/src/components/Nav.jsx:
##########
@@ -15,73 +15,104 @@
* limitations under the License.
*
*/
-import React from 'react'
+import React, { useContext, useEffect, useState, useCallback } from 'react'
import { Alignment, Position, Popover, Navbar, Icon } from '@blueprintjs/core'
import '@/styles/nav.scss'
import { ReactComponent as SlackIcon } from '@/images/slack-mark-monochrome-black.svg'
import { ReactComponent as SlackLogo } from '@/images/slack-rgb.svg'
+import UIContext from '@/store/UIContext'
+import useWindowSize from '@/hooks/useWIndowSize'
const Nav = () => {
+ const uiContext = useContext(UIContext)
+ const [menuClass, setMenuClass] = useState('navbarMenuButton')
+ const size = useWindowSize()
+
+ const toggleSidebarOpen = (open) => {
+ uiContext.changeSidebarVisibility(open)
+ setMenuClass((_) =>
+ open ? 'navbarMenuButtonSidebarOpened' : 'navbarMenuButton'
+ )
+ }
+
+ useEffect(
+ () =>
+ uiContext.desktopBreakpointWidth <= size.width
+ ? !uiContext.sidebarVisible && toggleSidebarOpen(true)
+ : toggleSidebarOpen(false),
+ [size]
+ )
+
return (
<Navbar className='navbar'>
- <Navbar.Group align={Alignment.RIGHT}>
- <a
- href='https://github.com/apache/incubator-devlake'
- rel='noreferrer'
- target='_blank'
- className='navIconLink'
- >
- <Icon icon='git-branch' size={16} />
- </a>
- <Navbar.Divider />
- <a
- href='mailto:hello@merico.dev'
- rel='noreferrer'
- target='_blank'
- className='navIconLink'
- >
- <Icon icon='envelope' size={16} />
- </a>
- <Navbar.Divider />
- {/* DISCORD: !DISABLED! */}
- {/* <a href='https://discord.com/invite/83rDG6ydVZ' rel='noreferrer' target='_blank' className='navIconLink'>
+ <Navbar.Group className={menuClass}>
+ <Icon
+ icon={uiContext.sidebarVisible ? 'menu-closed' : 'menu-open'}
+ onClick={toggleSidebarOpen.bind(null, !uiContext.sidebarVisible)}
Review Comment:
In order to resolve one of the ESLint Errors, this can be refactored to:
```suggestion
onClick={(e) => toggleSidebarOpen(!uiContext.sidebarVisible)}
```
##########
config-ui/src/components/Nav.jsx:
##########
@@ -15,73 +15,104 @@
* limitations under the License.
*
*/
-import React from 'react'
+import React, { useContext, useEffect, useState, useCallback } from 'react'
import { Alignment, Position, Popover, Navbar, Icon } from '@blueprintjs/core'
import '@/styles/nav.scss'
import { ReactComponent as SlackIcon } from '@/images/slack-mark-monochrome-black.svg'
import { ReactComponent as SlackLogo } from '@/images/slack-rgb.svg'
+import UIContext from '@/store/UIContext'
+import useWindowSize from '@/hooks/useWIndowSize'
const Nav = () => {
+ const uiContext = useContext(UIContext)
+ const [menuClass, setMenuClass] = useState('navbarMenuButton')
+ const size = useWindowSize()
+
+ const toggleSidebarOpen = (open) => {
+ uiContext.changeSidebarVisibility(open)
+ setMenuClass((_) =>
+ open ? 'navbarMenuButtonSidebarOpened' : 'navbarMenuButton'
+ )
+ }
+
+ useEffect(
+ () =>
+ uiContext.desktopBreakpointWidth <= size.width
+ ? !uiContext.sidebarVisible && toggleSidebarOpen(true)
+ : toggleSidebarOpen(false),
+ [size]
+ )
+
return (
<Navbar className='navbar'>
- <Navbar.Group align={Alignment.RIGHT}>
- <a
- href='https://github.com/apache/incubator-devlake'
- rel='noreferrer'
- target='_blank'
- className='navIconLink'
- >
- <Icon icon='git-branch' size={16} />
- </a>
- <Navbar.Divider />
- <a
- href='mailto:hello@merico.dev'
- rel='noreferrer'
- target='_blank'
- className='navIconLink'
- >
- <Icon icon='envelope' size={16} />
- </a>
- <Navbar.Divider />
- {/* DISCORD: !DISABLED! */}
- {/* <a href='https://discord.com/invite/83rDG6ydVZ' rel='noreferrer' target='_blank' className='navIconLink'>
+ <Navbar.Group className={menuClass}>
+ <Icon
+ icon={uiContext.sidebarVisible ? 'menu-closed' : 'menu-open'}
+ onClick={toggleSidebarOpen.bind(null, !uiContext.sidebarVisible)}
Review Comment:
@henit-chobisa In order to resolve one of the ESLint Errors, this can be refactored to:
```suggestion
onClick={(e) => toggleSidebarOpen(!uiContext.sidebarVisible)}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] e2corporation commented on pull request #3107: feat: Responsive sidebar
Posted by GitBox <gi...@apache.org>.
e2corporation commented on PR #3107:
URL: https://github.com/apache/incubator-devlake/pull/3107#issuecomment-1258432657
@henit-chobisa Additionally, the commit message structure for the last 2 commits does not conform to the current linter format (`fix:` `feat:` `refactor:` `chore:` etc) and is also causing the **Commit Message** CI Check to fail, can those commits messages be amended as well to fit the desired format?
<img width="577" alt="Screen Shot 2022-09-26 at 2 16 26 PM" src="https://user-images.githubusercontent.com/1742233/192351052-09c46ece-ec84-42f9-a70a-b48656493980.png">
**Expected format to pass CI Check**
- chore: removed makefile
- chore: removed blueprint.go
**or**
- refactor: removed makefile
- refactor: removed blueprint.go
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] e2corporation commented on pull request #3107: feat: Responsive sidebar
Posted by GitBox <gi...@apache.org>.
e2corporation commented on PR #3107:
URL: https://github.com/apache/incubator-devlake/pull/3107#issuecomment-1258451852
> > @henit-chobisa Additionally, the commit message structure for the last 2 commits does not conform to the current linter format (`fix:` `feat:` `refactor:` `chore:` etc) and is also causing the **Commit Message** CI Check to fail, can those commits messages be amended as well to fit the desired format?
> > <img alt="Screen Shot 2022-09-26 at 2 16 26 PM" width="577" src="https://user-images.githubusercontent.com/1742233/192351052-09c46ece-ec84-42f9-a70a-b48656493980.png">
> > **Expected format to pass CI Check**
> >
> > * chore: removed makefile
> > * chore: removed blueprint.go
> >
> > **or**
> >
> > * refactor: removed makefile
> > * refactor: removed blueprint.go
>
> I'm extremely sorry, I just forgot that I fix that now.
@henit-chobisa No problem, it takes a bit of a adjustment getting used to the Linter on your first PR. Subsequent PRs should be much smoother once you've gone through the process. General workflow is once you are code complete and ready to remove Draft state, `npm run lint` should be performed on `@config-ui` to resolve Lint errors and ensure the formatting standards are applied.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] henit-chobisa commented on pull request #3107: feat: Responsive sidebar
Posted by GitBox <gi...@apache.org>.
henit-chobisa commented on PR #3107:
URL: https://github.com/apache/incubator-devlake/pull/3107#issuecomment-1260372761
Okay sure @e2corporation, I do the same.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] henit-chobisa commented on pull request #3107: feat: Responsive sidebar
Posted by GitBox <gi...@apache.org>.
henit-chobisa commented on PR #3107:
URL: https://github.com/apache/incubator-devlake/pull/3107#issuecomment-1252721336
Thank You @e2corporation for such a detailed Review,
I will cover up on all your points and push the fixtures by tomorrow EOD
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] henit-chobisa commented on pull request #3107: feat: Responsive sidebar
Posted by GitBox <gi...@apache.org>.
henit-chobisa commented on PR #3107:
URL: https://github.com/apache/incubator-devlake/pull/3107#issuecomment-1257033101
@e2corporation
Julien, I have worked on what you said and now I think that the branch is up to date with master.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] henit-chobisa commented on pull request #3107: feat: Responsive sidebar
Posted by GitBox <gi...@apache.org>.
henit-chobisa commented on PR #3107:
URL: https://github.com/apache/incubator-devlake/pull/3107#issuecomment-1255419358
Ok sure, understood
I will rebase the branch and cherry pick the changes!
Thank You for keeping up with me and guiding me at every step.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] e2corporation commented on pull request #3107: feat: Responsive sidebar
Posted by GitBox <gi...@apache.org>.
e2corporation commented on PR #3107:
URL: https://github.com/apache/incubator-devlake/pull/3107#issuecomment-1258457125
@henit-chobisa Another issue that may affect this PR from a clean merge is that you have issued `merge` commits when updating changes from `main` branch, when we prefer a `rebase` policy instead (ie `git pull --rebase`) that way feature commits are replayed on top of main cleanly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] henit-chobisa commented on pull request #3107: feat: Responsive sidebar
Posted by GitBox <gi...@apache.org>.
henit-chobisa commented on PR #3107:
URL: https://github.com/apache/incubator-devlake/pull/3107#issuecomment-1255368846
@e2corporation
Julien, would it be fine if I go with resolving the conflict, are there any downsides to that ?
<img width="923" alt="image" src="https://user-images.githubusercontent.com/72302948/191818737-6916f460-907e-4ab6-b322-668697b627af.png">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] e2corporation commented on a diff in pull request #3107: feat: Responsive sidebar
Posted by GitBox <gi...@apache.org>.
e2corporation commented on code in PR #3107:
URL: https://github.com/apache/incubator-devlake/pull/3107#discussion_r977926951
##########
config-ui/src/index.js:
##########
@@ -20,8 +20,11 @@ import React from 'react'
import ReactDOM from 'react-dom'
import App from './App'
+import { UIContextProvider } from './store/UIContext'
Review Comment:
```suggestion
import { UIContextProvider } from '@/store/UIContext'
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] e2corporation commented on pull request #3107: feat: Responsive sidebar
Posted by GitBox <gi...@apache.org>.
e2corporation commented on PR #3107:
URL: https://github.com/apache/incubator-devlake/pull/3107#issuecomment-1252688198
@henit-chobisa Thanks again for the Responsive Sidebar Feature Henit, I'll be adding some review notes, in the meantime here's a few points around the existing coding standards you can start applying. We recently added Prettier + ESLint configuration to better assist with this, for now you may make the adjustments manually.
- [ ] We have adopted a **No semi-colons** Policy (Please strip all semicolons)
- [ ] _Single_ Quotes **TRUE** (No Double-quotes, except in ASF Headers)
- [ ] Tab Spaces **2**
- [ ] Convert Tabs to Spaces **TRUE**
- [ ] Trailing Commas **No** (Not Preferred)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] e2corporation commented on a diff in pull request #3107: feat: Responsive sidebar
Posted by GitBox <gi...@apache.org>.
e2corporation commented on code in PR #3107:
URL: https://github.com/apache/incubator-devlake/pull/3107#discussion_r975652613
##########
config-ui/src/components/Nav.jsx:
##########
@@ -26,11 +26,41 @@ import {
import '@/styles/nav.scss'
import { ReactComponent as SlackIcon } from '@/images/slack-mark-monochrome-black.svg'
import { ReactComponent as SlackLogo } from '@/images/slack-rgb.svg'
+import UIContext from '../store/UIContext'
+import useWindowSize from '../hooks/useWIndowSize'
+
const Nav = () => {
+ const uiContext = useContext(UIContext);
+ const [menuClass, setMenuClass] = useState("navbarMenuButton");
+ const size = useWindowSize();
+
+ const toggleSidebarOpen = (open) => {
+ uiContext.changeSidebarVisibility(open)
+ if (open){
+ setMenuClass("navbarMenuButtonSidebarOpened");
+ return;
+ }
+ setMenuClass("navbarMenuButton");
+ }
+
+ useEffect(() => {
+ if (size.width >= 850 ) {
Review Comment:
Would it be beneficial to store the `850` breakpoint value as `desktopBreakpointWidth` within the UIContext's state object so it can be configurable?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] e2corporation commented on a diff in pull request #3107: feat: Responsive sidebar
Posted by GitBox <gi...@apache.org>.
e2corporation commented on code in PR #3107:
URL: https://github.com/apache/incubator-devlake/pull/3107#discussion_r975648117
##########
config-ui/src/components/Nav.jsx:
##########
@@ -26,11 +26,41 @@ import {
import '@/styles/nav.scss'
import { ReactComponent as SlackIcon } from '@/images/slack-mark-monochrome-black.svg'
import { ReactComponent as SlackLogo } from '@/images/slack-rgb.svg'
+import UIContext from '../store/UIContext'
+import useWindowSize from '../hooks/useWIndowSize'
+
const Nav = () => {
+ const uiContext = useContext(UIContext);
+ const [menuClass, setMenuClass] = useState("navbarMenuButton");
+ const size = useWindowSize();
+
+ const toggleSidebarOpen = (open) => {
Review Comment:
I think this could be refactored to be more declarative and drop the need for using the `IF` and `return` statements.
```suggestion
const toggleSidebarOpen = (open) => {
uiContext.changeSidebarVisibility(open)
setMenuClass(open ? 'navbarMenuButtonSidebarOpened' : 'navbarMenuButton'
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] e2corporation commented on pull request #3107: feat: Responsive sidebar
Posted by GitBox <gi...@apache.org>.
e2corporation commented on PR #3107:
URL: https://github.com/apache/incubator-devlake/pull/3107#issuecomment-1258061193
@henit-chobisa Thanks for the updates Henit, it does look like we have some unwanted **backend** files on this PR that will need to be removed. Can these resources be reset to `HEAD` version from `main` so they are discarded from this PR?
- `Makefile`
- `plugins/`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] henit-chobisa commented on pull request #3107: feat: Responsive sidebar
Posted by GitBox <gi...@apache.org>.
henit-chobisa commented on PR #3107:
URL: https://github.com/apache/incubator-devlake/pull/3107#issuecomment-1259810554
> > > @henit-chobisa Additionally, the commit message structure for the last 2 commits does not conform to the current linter format (`fix:` `feat:` `refactor:` `chore:` etc) and is also causing the **Commit Message** CI Check to fail, can those commits messages be amended as well to fit the desired format?
> > > <img alt="Screen Shot 2022-09-26 at 2 16 26 PM" width="577" src="https://user-images.githubusercontent.com/1742233/192351052-09c46ece-ec84-42f9-a70a-b48656493980.png">
> > > **Expected format to pass CI Check**
> > >
> > > * chore: removed makefile
> > > * chore: removed blueprint.go
> > >
> > > **or**
> > >
> > > * refactor: removed makefile
> > > * refactor: removed blueprint.go
> >
> >
> > I'm extremely sorry, I just forgot that I fix that now.
>
> @henit-chobisa No problem, it takes a bit of a adjustment getting used to the Linter on your first PR. Subsequent PRs should be much smoother once you've gone through the process. General workflow is once you are code complete and ready to remove Draft state, `npm run lint` should be performed on `@config-ui` to resolve Lint errors and ensure the formatting standards are applied.
Sure Julien, I will take care that I won't create this much issues for reviewers and do everything perfectly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] henit-chobisa commented on pull request #3107: feat: Responsive sidebar
Posted by GitBox <gi...@apache.org>.
henit-chobisa commented on PR #3107:
URL: https://github.com/apache/incubator-devlake/pull/3107#issuecomment-1259814523
> @henit-chobisa Another issue that may affect this PR from a clean merge is that you have issued `merge` commits when updating changes from `main` branch, when we prefer a `rebase` policy instead (ie `git pull --rebase`) that way feature commits are replayed on top of main cleanly.
So should I undo the merge commits and do rebase with that ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] e2corporation commented on pull request #3107: feat: Responsive sidebar
Posted by GitBox <gi...@apache.org>.
e2corporation commented on PR #3107:
URL: https://github.com/apache/incubator-devlake/pull/3107#issuecomment-1259949890
> > > > @henit-chobisa Additionally, the commit message structure for the last 2 commits does not conform to the current linter format (`fix:` `feat:` `refactor:` `chore:` etc) and is also causing the **Commit Message** CI Check to fail, can those commits messages be amended as well to fit the desired format?
> > > > <img alt="Screen Shot 2022-09-26 at 2 16 26 PM" width="577" src="https://user-images.githubusercontent.com/1742233/192351052-09c46ece-ec84-42f9-a70a-b48656493980.png">
> > > > **Expected format to pass CI Check**
> > > >
> > > > * chore: removed makefile
> > > > * chore: removed blueprint.go
> > > >
> > > > **or**
> > > >
> > > > * refactor: removed makefile
> > > > * refactor: removed blueprint.go
> > >
> > >
> > > I'm extremely sorry, I just forgot that I fix that now.
> >
> >
> > @henit-chobisa No problem, it takes a bit of a adjustment getting used to the Linter on your first PR. Subsequent PRs should be much smoother once you've gone through the process. General workflow is once you are code complete and ready to remove Draft state, `npm run lint` should be performed on `@config-ui` to resolve Lint errors and ensure the formatting standards are applied.
>
> Sure Julien, I will take care that I won't create this much issues for reviewers and do everything perfectly.
Thanks Henit, it's ok if issues occur, this is why we have the code review process.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] e2corporation commented on a diff in pull request #3107: feat: Responsive sidebar
Posted by GitBox <gi...@apache.org>.
e2corporation commented on code in PR #3107:
URL: https://github.com/apache/incubator-devlake/pull/3107#discussion_r975649772
##########
config-ui/src/components/Nav.jsx:
##########
@@ -26,11 +26,41 @@ import {
import '@/styles/nav.scss'
import { ReactComponent as SlackIcon } from '@/images/slack-mark-monochrome-black.svg'
import { ReactComponent as SlackLogo } from '@/images/slack-rgb.svg'
+import UIContext from '../store/UIContext'
+import useWindowSize from '../hooks/useWIndowSize'
+
const Nav = () => {
+ const uiContext = useContext(UIContext);
+ const [menuClass, setMenuClass] = useState("navbarMenuButton");
+ const size = useWindowSize();
+
+ const toggleSidebarOpen = (open) => {
+ uiContext.changeSidebarVisibility(open)
+ if (open){
+ setMenuClass("navbarMenuButtonSidebarOpened");
+ return;
+ }
+ setMenuClass("navbarMenuButton");
+ }
+
+ useEffect(() => {
Review Comment:
This Effect could also be refactored to be more declarative and remove the nested `IF` statements
```suggestion
useEffect(() => {
toggleSidebarOpen(size.width >= 850 && uiContext.sidebarVisible != true ? true : false)
}, [size]);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] e2corporation commented on a diff in pull request #3107: feat: Responsive sidebar
Posted by GitBox <gi...@apache.org>.
e2corporation commented on code in PR #3107:
URL: https://github.com/apache/incubator-devlake/pull/3107#discussion_r975654586
##########
config-ui/src/hooks/useWIndowSize.jsx:
##########
@@ -0,0 +1,29 @@
+import { useEffect, useState } from "react";
+
+function useWindowSize() {
+ const [windowSize, setWindowSize] = useState({
Review Comment:
Would it be better to initialize these to integer values rather than undefined, or is undefined desired? Maybe even a min-viewport size of 320x240.
```suggestion
const [windowSize, setWindowSize] = useState({
width: 320,
height: 240,
})
```
##########
config-ui/src/hooks/useWIndowSize.jsx:
##########
@@ -0,0 +1,29 @@
+import { useEffect, useState } from "react";
+
+function useWindowSize() {
+ const [windowSize, setWindowSize] = useState({
Review Comment:
Would it be better to initialize these to integer values rather than undefined, or is undefined desired? Maybe even a min-viewport size of 320x240.
```suggestion
const [windowSize, setWindowSize] = useState({
width: 320,
height: 240
})
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] e2corporation commented on pull request #3107: feat: Responsive sidebar
Posted by GitBox <gi...@apache.org>.
e2corporation commented on PR #3107:
URL: https://github.com/apache/incubator-devlake/pull/3107#issuecomment-1255378763
> @e2corporation Julien, would it be fine if I go with resolving the conflict, are there any downsides to that ? <img alt="image" width="923" src="https://user-images.githubusercontent.com/72302948/191818737-6916f460-907e-4ab6-b322-668697b627af.png">
I'm not sure if/how Github will auto-resolve the conflicts successful given the nature of the recent changes, so resolving the conflicts may still end up being a manual process.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org