fix(v2): responsive layouts based on width of drawer #4836
Merged
Conversation
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
karrui
approved these changes
Sep 12, 2022
...ilder-and-design/BuilderAndDesignDrawer/EditFieldDrawer/edit-fieldtype/EditDate/EditDate.tsx
Outdated
Show resolved
Hide resolved
darrelhong
force-pushed
the
form-v2/listen-drawer-width
branch
from
6e964c9 to
4e5fd5c
Compare
darrelhong
changed the title
timotheeg
reviewed
Sep 14, 2022
Comment on lines
28
to
27
| const { useMeasureDrawer } = useCreatePageSidebar() | ||
| const [drawerRef] = useMeasureDrawer | ||
|
|
There was a problem hiding this comment.
This can probably be merged into line 25 instead:
const { activeTab, isDrawerOpen, useMeasureDrawer: [drawerRef] } = useCreatePageSidebar()There was a problem hiding this comment.
tunnel vision got me, fixed. kept it as 2 lines as its a tad more readable imo
darrelhong
force-pushed
the
form-v2/listen-drawer-width
branch
from
4e5fd5c to
69eb381
Compare
darrelhong
requested review from
timotheeg and
karrui
and removed request for
karrui and
timotheeg
karrui
reviewed
Sep 14, 2022
| @@ -151,6 +154,8 @@ export const useCreatePageSidebarContext = | |||
| setPendingTab(undefined) | |||
| }, [isMobile, pendingTab, setFieldsToInactive]) | |||
|
|
|||
| const useMeasureDrawer = useMeasure() | |||
There was a problem hiding this comment.
nit: we should not prefix non-hooks with use keyword, causes unnecessary confusion. should we call this drawerDimensions instead?
There was a problem hiding this comment.
seems like we only need the drawer width too; suggest just returning drawerWidth from this hook instead of the entire dimensions.
There was a problem hiding this comment.
agree on both points
darrelhong
force-pushed
the
form-v2/listen-drawer-width
branch
from
69eb381 to
681869c
Compare
This was referenced Oct 4, 2022
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Problem
breakpoints might not be flexible enough as the current layout allows a split drawer and content up to minimum width of 768px which means each column can get very narrow. Since breakpoints are based on the width of the entire page, it can't account for the split panels. This mainly affects the custom date range input in form builder.
Solution
Most scenarios can be satisfied by simply setting breakpoints at
lginstead ofmd, as per #4835. This PR adds a hook that watches the width of the drawer container then layouts can be changed at any arbitrary width.Pros:
More flexible
Cons:
Not the cleanest code
Room for improvement:
hook could probably be optimised to give it more structure as opposed to returning the raw width?
Before & After Screenshots
BEFORE
WITH BREAKPOINTS
WITH ARBITRARY WIDTH
