Skip to content
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

Fix medex navbar overflow #3732

Merged
merged 4 commits into from Aug 10, 2020
Merged

Conversation

stu01509
Copy link
Member

@stu01509 stu01509 commented Jul 10, 2020

Short description of what this resolves:

Fix the medex navbar overflow.

Before:
image

After:
image

Copy link
Contributor

@tywrenn tywrenn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stu01509 This navbar isn't displaying correctly on a mobile device. Probably need to fix that as well.

@stu01509
Copy link
Member Author

Update the commit.

In Firefox:
image

image

In chrome:
image

image

Comment on lines 1524 to 1527
<div id="hide_nav" style="<?php if ($setting_bootstrap_submenu == 'hide') {
<div id="hide_nav" class="mt-3" style="<?php if ($setting_bootstrap_submenu == 'hide') {
echo "display:none;"; } ?>">
<nav id="navbar_oe" class="bgcolor2 fixed-top navbar-expand-md navbar-custom navbar-bright navbar-inner" name="kiosk_hide" data-role="page banner navigation">
<!-- Brand and toggle get grouped for better mobile display -->
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @tywrenn and @bradymiller
In my opinion, this arrow button is unnecessary, because the navbar already has the toggle to collapse or show.
What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, it seems like an outdated measure for mobile devices that we used to do. You can go ahead and remove it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tywrenn update the commit, please take a look.

@tywrenn
Copy link
Contributor

tywrenn commented Aug 9, 2020

@stu01509 Please rebase this to master

@stu01509
Copy link
Member Author

stu01509 commented Aug 9, 2020

Hi @tywrenn

Please check again :)

@tywrenn tywrenn self-requested a review August 10, 2020 02:45
Copy link
Contributor

@tywrenn tywrenn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sjpadgett
Copy link
Sponsor Member

I think Brady is working on patch 4. If you're okay with this, i'll bring in to prevent conflicts with other ongoing PRs..

@sjpadgett sjpadgett merged commit 691c788 into openemr:master Aug 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants