This repository was archived by the owner on Mar 4, 2020. It is now read-only.
fix(Popup): allow to 'detach' from trigger and RTL adjustments #612
Conversation
This file contains hidden or 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
…ardust-ui/react into fix/popup-offset-values-range
| expectOffsetTransformResult('100%', '-100%') | ||
| expectOffsetTransformResult(' 2000%p ', '-2000%p') | ||
| expectOffsetTransformResult('100 ', '-100') | ||
| expectOffsetTransformResult(' - 200vh', '200vh') |
There was a problem hiding this comment.
You can use jest.each there: https://jestjs.io/docs/en/api#testeachtable-name-fn-timeout
There was a problem hiding this comment.
have intentionally selected this approach for the following reasons:
- the
jest.each()is less expressive than explicit name of the check - we are not interested in
namearg thatjest.each()might provide for each case under check - even given that this is more verbose, might argue that we will read these lines far more often than write them, so I'd better prefer expressiveness here
- this approach is consistent with the one that is already taken for the rest of the tests in this file (which is, also, provide more readable tests code)
| .replace(/<minus>/g, '-') | ||
| .trimLeft() | ||
| .replace(/^\+/, '') | ||
| } |
There was a problem hiding this comment.
This looks a complicated for me, but we have tests. Possibly, we can to add memoization there.
There was a problem hiding this comment.
yes, the tests were introduced exactly for this sake :) not sure about memoization, as all expressions used are of linear complexity - there shouldn't be any significant gain from it.
| key={`${position}-${align}`} | ||
| /> | ||
| ))} | ||
| <Grid columns="repeat(1, 80px)" variables={{ padding: '30px', gridGap: '30px' }}> |
There was a problem hiding this comment.
It seems that we don't repeat there more.
There was a problem hiding this comment.
absolutely, need to fix
layershifter
approved these changes
Dec 18, 2018
mnajdova
pushed a commit
that referenced
this pull request
Dec 19, 2018
* introduce offset prop * correct description of supported values * update changelog * introduce fix * ensure RTL is properly applied to complex offset expressions * rename method to make logic more expressive * add unit tests * remove unnecessary grid props from offset example * update changelog
mnajdova
pushed a commit
that referenced
this pull request
Dec 19, 2018
* feat(menuItem: add menu prop) * Only one submenu open at a time * Fix bug: submenu noe closes on clicking a menuItem with no submenu * Code cleanup * Remove the activeIndex prop passed to the menuItem * Remove Popup and implement submenu without it * initial keyboard support * left/right arrow handling * simplify keyboard handlers * -fixed import in the MenuItem * -added submenuIndicator -small fixes in the styles and the way the submenu is generated * -clicking on leaf element should close the submenus (the same should be done for enter/space) -applied consistent (left-right) navigation for horizontal menu and (up-down) navigation for vertical menu * -implemented outside click to close all menus -implemented enter key on leaf menu item to close the menu * -fixed import * -refactored MenuItem handlers - fixed issues -removed onClick handler for the Menu (not necessary for now) -added onKeyDown in the creation of the MenuItem in the Menu component for handling the action prop * -added setActiveIndex callback and removed onKeyDown in the creation of MenuItems in the Menu component * -right arrow key is closing the submenus and goes to the next element if the menu is horizontal, or is focusing the first MenuItem if it is vertical * -handled left arrow key * -changed ref -focus trap wip * -added Ref component instead of using the itemRef on the ElementType -removed subscription for focus * -fixes * -moved ref //TODO: figure out tests failing * -close menu on outside focus * -improved comments * -fixed escape key not focusing the active element -changed parentRef to inSubmenu boolean * -fixing key problems -added dependency for generating id * -refactored submenuRef element * fix broken tests * -added comments in the tests -changed the submenuDomElement so submenuRef * -renamed inSubmenu to submenu prop in the Menu * -fixed with the auto-controlled prop in the Menu * -added state interface in the Menu -improved menu variables' names * -fixed variables in examples * -remove state initialization in the MenuItem component * -added new handler for escape -changed submenu examples titles * -refactored conditions using doesNodeContainClick * -renamed submenu* props to menu in the MenuItem component -changed setActiveIndex with onActiveChanged -introduced different styles for the hovering vs active elements * -improved example -fixed issue with the condition for the active prop * -exported MenuState -added correct typings to the menuStyles * -fixed underlined active + hovered style * -fixed border corner clipped by adding custom styles for the first child menu items and the last child menu items in vertical menu * -addressed comments on PR * chore: prepare release 0.15.0 [ci skip] * 0.15.0 * fix(Prototype): Fix Popover prototype after breaking changes (#623) * chore: cache results of vulnerability scans (#621) * implement caching strategy * adjust file name of scan marker * add yarn lock hash to marker file name * add change to build config * fix dir name in build config * improve caching strategy * just restore cache * temporary remove lint and tests * try * fix caching strategy * try * try * try * try epoch * create file on scan * return lint and test steps * introduce comment for the caching approach taken * remove unnecessary function * simplify expression for marker file name * feat(text): color prop (#597) * feat(text): color prop * addressed comments * changelog * amended changelog * made text color override other props that change color * feat(header): header and header description color prop (#628) * feat(header): header and header description color prop * changelog * fixed examples * addressed PR comments * fix(Popup): allow to 'detach' from trigger and RTL adjustments (#612) * introduce offset prop * correct description of supported values * update changelog * introduce fix * ensure RTL is properly applied to complex offset expressions * rename method to make logic more expressive * add unit tests * remove unnecessary grid props from offset example * update changelog * fix(SelectableList): Items in list should be selectable (#566) * Reflect which item is selected in list * Make list derived from autocontrolled component * small fix * Update ListExampleSelection.tsx * Update ListExampleSelection.shorthand.tsx * Small improvement * Rename *ItemIndex -> *Index * Names refactoring * Minor improvements * update changelog * Add onSelectedIndexChange * Add some tests * Small improvements afer CR * Small improvements afer CR * Small improvements afer CR * create focus handler when List is constructed * fix changelog * changelog * docs(Examples): allow to use TS in examples (#617) * docs(Examples): allow to use TS in examples * add jsdoc * fix typo * rename file * add comment * `createExample` to `createExampleSourceCode` * rework with `path.relative()` * remove JSON files on remove tsx * create getRelativePathToSource function * -updated changelog * -updated changelog * -updated changelog with breaking changes -removed TODO comment * -fixed imports in examples
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
TODO
This PR fixes the following issues: #606 (comment)
Allow to detach popup from trigger
Previously it was impossible - it was guaranteed by implementation logic that popup and trigger would always have at least single common point. Thus, even for the following example where user might intentionally would like to specify large offset value - even in that case rendered popup will be 'attached' to its trigger.
with an extremely huge offset result had stayed the same:
Provided fix ensures that client's offset will be applied 'as is', without any additional heuristics being applied.
Ensure correct RTL transform for complex
offsetexpressionsPrevious RTL transform logic had implicitly assumed that it could be only single value passed as
offset- however,offsetalso supports expressions syntax, so that the following value is absolutely correct:This PR addresses such limitation.