fix(enhancers): Made enhancers position dependent on where you click enhancers from, if mobile, setting floating to true. hide scrollbars in chrome #286
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
KemoPaw
approved these changes
Nov 19, 2021
Jekrimo
approved these changes
Nov 22, 2021
| /* Prevent toggle twice when toggle enhancer button is clicked (for mobile case ) */ | ||
| const chatToggleEnhancer = document.getElementById('chat-enhancer-toggle') | ||
| // @ts-ignore | ||
| // // @ts-ignore |
There was a problem hiding this comment.
Can we just delete this and the this.click above?
There was a problem hiding this comment.
If possible on this, we should use refs, using document selectors is an antipattern in vue.
| /* Prevent toggle twice when toggle enhancer button is clicked (for mobile case ) */ | ||
| const chatToggleEnhancer = document.getElementById('chat-enhancer-toggle') | ||
| // @ts-ignore | ||
| // // @ts-ignore |
There was a problem hiding this comment.
If possible on this, we should use refs, using document selectors is an antipattern in vue.
| @@ -26,6 +26,10 @@ const InitalUIState = (): UIState => ({ | |||
| enhancers: { | |||
| show: false, | |||
| floating: false, | |||
| position: [0, 0], | |||
| defaultWidth: '24rem', | |||
| defaultHeight: '30rem', | |||
There was a problem hiding this comment.
I don't love storing these values in state, can we use global less vars here? Do these need to be dynamic? IF so can we use less selectors for it?
There was a problem hiding this comment.
We moved these to state because we need them in the style and for a calculation to detect if the enhancers component would be off screen
stavares843
added
the
Changes Requested
InfamousVague
approved these changes
Nov 24, 2021
WanderingHogan
force-pushed
the
sa-187
branch
2 times, most recently
from
4b16666
to
97ab546
Compare
…from, if mobile, setting floating to true. hide scrollbars in chrome. #SA-187 and #AP-112.
WanderingHogan
force-pushed
the
sa-187
branch
from
97ab546
to
b20a33c
Compare
github-actions
bot
removed
the
Changes Requested
InfamousVague
pushed a commit
that referenced
this pull request
Dec 5, 2021
…from, if mobile, setting floating to true. hide scrollbars in chrome. #SA-187 and #AP-112. (#286)
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.
What this PR does 📖
These changes make the enhancers menu show up close to where the user clicks the enhancer from on desktop. On mobile it defaults to a floating enhancer. This also removes the chrome scrollbar for the enhancer.
Which issue(s) this PR fixes 🔨
Fixes # SA-187
Special notes for reviewers 🗒️
Tested on desktop and through mobile view in chrome devtools. Reload in between aspect ratio/form factor changes. This is mostly taken from the @KemoPaw's recent PR
Additional comments 🎤