Skip to content
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

Feat/added isVisible to page props #1262

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

acamara2016
Copy link

@acamara2016 acamara2016 commented Sep 23, 2022

This PR is to allow hiding/reveal for custom components nav from the dashboard. Unlike the resources, custom components cannot be hidden from the dashboard

@acamara2016 acamara2016 changed the title added isShow to page props Feat/added isShow to page props Sep 23, 2022
@@ -55,19 +55,19 @@ export interface AdminJSOptions {
* path, under which, AdminJS will be available. Default to `/admin`
*
*/
rootPath?: string;
rootPath?: string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why irrelevant changes?

/**
* Page visibility
*/
isShown?: boolean
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not isVisible? That's what we use in other configs, this could also be boolean | IsFunction

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed isShown to isVisible in the latest commit with type either boolean | IsFunction

@@ -17,36 +17,29 @@ const SidebarPages: React.FC<Props> = (props) => {

const { translateLabel } = useTranslation()
const location = useLocation()
const navigate = useNavigate()
const history = useHistory()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we've updated to react-router v6 which doesn't export useHistory, it has useNavigate instead


if (!pages || !pages.length) {
return null
return <></>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

returning an empty fragment is slower than returning null as it creates a DOM element, I don't see a reason for this change

@acamara2016 acamara2016 force-pushed the Issue#43/Adding-option-to-hide-custom-page-nav-in-dashboard branch from 2800a8c to ad6b8f0 Compare September 28, 2022 17:56
@acamara2016 acamara2016 changed the title Feat/added isShow to page props Feat/added isVisible to page props Sep 28, 2022
@acamara2016 acamara2016 force-pushed the Issue#43/Adding-option-to-hide-custom-page-nav-in-dashboard branch from ad6b8f0 to df2a541 Compare September 28, 2022 18:02
@acamara2016 acamara2016 force-pushed the Issue#43/Adding-option-to-hide-custom-page-nav-in-dashboard branch from df2a541 to daea8d6 Compare September 28, 2022 18:05
@acamara2016 acamara2016 requested a review from dziraf September 28, 2022 18:05
/**
* Page visibility
*/
isVisible: boolean | IsFunction;
Copy link
Contributor

@dziraf dziraf Sep 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PageJSON is something returned to the frontend from the backend so it cannot be a function (IsFunction) and has to be pre-evaluated.

pagesToStore would have to do something like:

  Object.entries(pages).map(([key, adminPage]) => {
      let isVisible = true;
      if (adminPage.isVisible) {
         if (typeof adminPage.isVisible === 'function') {
           isVisible = adminPage.isVisible({ currentAdmin })
         } else {
           isVisible = adminPage.isVisible
         }
      }
      return {
        name: key,
        component: adminPage.component,
        icon: adminPage.icon,
        isVisible,
      }
  })

@acamara2016 acamara2016 requested a review from dziraf October 2, 2022 17:52
Copy link
Contributor

@dziraf dziraf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last thing, have you tested if this actually hides the page from the sidebar?

I'm not sure if any changes here are required: https://github.com/SoftwareBrothers/adminjs/blob/5145c48b7eb0758a603a4ee5435aa3883193ddd5/src/frontend/components/app/sidebar/sidebar-pages.tsx

@@ -1,3 +1,6 @@
import { IsFunction } from 'src/backend'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I haven't noticed this earlier. Can you change the import path to be relative (not start with src/)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants