-
Notifications
You must be signed in to change notification settings - Fork 17
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(command-palette): grid layout #1671
base: feature-command-palette
Are you sure you want to change the base?
Conversation
- use native dialog element for modal container - separate modal logic to a useModal hook - add a constants file for commonly used strings
- remove functionality creating focus on 2 items at once by keyboard and hovering - update test simulating focus by hovering over an item
- add key press handlers for each arrow key
- add test to search for logout on home view - add filterable_action type - add actions to all items that can be filtered via the useFilter hook - remove setup for commands
* feat: update esc, tab and backspace keydown logic * feat: override default behaviour for cmd+k shortcut * fix: input component import * fix: open apps in same window
🚀 Deployed on https://pr-1671--dhis2-ui.netlify.app |
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.
Made a few code cleanup suggestions - using useMemo
is better than useEffect
here because it avoids an additional re-render
const apps = itemsArray?.filter((item) => item.type === APP) | ||
const expectedGridSize = gridColumns * gridRows | ||
const appsGridSize = | ||
apps.length >= expectedGridSize | ||
? expectedGridSize | ||
: apps.length |
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 logic is repeated, perhaps it should be refactored or passed as an argument directly?
useEffect(() => { | ||
const columns = | ||
gridLayout === 'desktop' | ||
? GRID_COLUMNS_DESKTOP | ||
: GRID_COLUMNS_MOBILE | ||
const rows = | ||
gridLayout === 'desktop' ? GRID_ROWS_DESKTOP : GRID_ROWS_MOBILE | ||
|
||
const expectedGridSize = rows * columns | ||
const availableApps = | ||
apps?.length >= expectedGridSize ? expectedGridSize : apps.length | ||
|
||
if (availableApps && availableApps < expectedGridSize) { | ||
if (availableApps / columns < 1) { | ||
setGridColumns(availableApps) | ||
setGridRows(1) | ||
} else { | ||
if (availableApps % columns === 0) { | ||
setGridColumns(columns) | ||
setGridRows(availableApps / columns) | ||
} else { | ||
setGridColumns(columns) | ||
setGridRows(Math.trunc(availableApps / columns + 1)) | ||
} | ||
} | ||
} else { | ||
setGridColumns(columns) | ||
setGridRows(rows) | ||
} | ||
}, [apps, gridLayout]) |
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 this can all be rewritten as a single useMemo
instead of 2 additional state variables - the function takes apps
and gridLayout
(maybe with a different name?) and returns an object { rows: x, columns: y }
Then call it with a useMemo
:
const rowsAndColumns = useMemo(() => getRowsAndColumns(apps, gridLayout), [apps, gridLayout])
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 feel like gridLayout
is actually a good name for the full rows/columns/totalCount object and maybe what's currently called gridLayout
should instead be something like formFactor
or isMobile
or something? Not a huge issue
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.
It might make sense to have a Grid
class instead of a bunch of helper functions?
41a0091
to
20aa21f
Compare
|
Addresses LIBS-752
Description
This PR adjusts the grid layout on the
HomeView
depending on the screen size.width < 480
, it shows a2 * 4
grid (columns * rows).4 * 2
grid is shown.It also adjusts the layout and navigation accordingly depending on how many apps are available in the grid.
Checklist
Screenshots
full.grid.-.small.screen.mov
partial.grid.-.small.screen.mov
partial.grid.-.large.screens.mov