(feat) Anchor workspace actions to the bottom of the screen in tablet mode #1650
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
donaldkibet
requested review from
denniskigen,
brandones and
ibacher
and removed request for
denniskigen and
brandones
|
Size Change: +228 B (0%) Total Size: 10.7 MB ℹ️ View Unchanged
|
makombe
approved these changes
Feb 9, 2024
There was a problem hiding this comment.
Thanks @donaldkibet . I approve this so that we can test it more with the pilot sites
ibacher
reviewed
Feb 9, 2024
Comment on lines
-96
to
+99
| height: var(--desktop-workspace-window-height); | ||
| // height: var(--desktop-workspace-window-height); |
There was a problem hiding this comment.
Can we fix this, since this has been merged?
Comment on lines
+13
to
14
| const isTablet = useLayoutType() === 'tablet'; | ||
| useEffect(() => { |
There was a problem hiding this comment.
Would've been nice to keep the newline here...
| @@ -77,7 +77,7 @@ const PatientChart: React.FC = () => { | |||
| </> | |||
| )} | |||
| </div> | |||
| <ActionMenu open={false} /> | |||
| <ActionMenu open={active} /> | |||
There was a problem hiding this comment.
I'm a little confused on the logic here? The action menu is open if the workspace is active? It feels like we might've needed a different prop for this.
There was a problem hiding this comment.
@ibacher It seems I may have misinterpreted the original idea. The intention is to conceal the action menu on tablet devices when a workspace form is open, allowing for increased working space on smaller tablets. I will do a follow on PR to address it.
|
The CSS change was actually meant to be done in the workspace-window.component rather than individual workspaces |
3 tasks
|
The PR description of this PR was totally different than what the PR did. It has no screenshots despite being a major UI change; PRs must have screenshots if they introduce UI changes. And what the PR did was to make the app disagree with the designs for everyone. This PR is a major regression, pushed through with no respect for our process, and should not have happened. |
|
@brandones - I remember we had this conversation a few months ago and we explained what necessitated this particular PR. At the time, we thought never needed forks of the ESMs and so relied on the community versions. We learned from the experience and we've since adjusted well. In addition, we needed this change for the tablet/small screen users since the implementation then was totally unusable. I propose we review both the solution and the designs so that we find a lasting solution if at all it wasn't done to an agreed expectation. @donaldkibet @makombe |
|
That's all fine, @ojwanganto . The problem is that none of this was documented anywhere. The title and description of this PR don't describe what it does. There are no screenshots: there were no screenshots of the bugs you describe, and no screenshots of what this fix does. And so no one noticed that this is contradictory to the designs. If we need to fix the designs because they don't actually work, based on your testing, then that's really important to do, and really helpful information to have from your team. I'm grateful that you all are using it on tablets & small screens and figuring out the problems. But that requires screenshots and correct documentation of changes. Without that, the conversation can't even happen. Of course we want to ensure the app is usable for everyone. But I'm redoing the workspace now, and the lack of documentation and screenshots makes it difficult for me to figure out how your team wants it to work. But I will do my best, and there will be a config option that supports this. |
|
Oh right, here's this one demonstrating the problem, for posterity: #1666 (comment) . And then here is a ticket for the problem: https://openmrs.atlassian.net/browse/O3-2946 |
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.
Requirements
Summary
This fixes an issue with tablet not showing the action buttons on tablet devices. It anchors the action buttons at the bottom.
Screenshots
Related Issue
Other