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

hide ple-footer when typing on small screen #464

Closed
wants to merge 8 commits into from

Conversation

Shulammite-Aso
Copy link
Collaborator

fixes #435
When typing on small screens the ple-footer disappears making more space for user to be able to see what they are typing.
Instead of this:
1586552649381

we'll have this:
Peek 2020-04-10 20-54

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with grunt jasmine
  • code is in uniquely-named feature branch and has no merge conflicts
  • PR is descriptively titled
  • PR body includes fixes #0000-style reference to original issue #
  • ask @publiclab/reviewers for help, in a comment below

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

We have a loose schedule of reviewing and pulling in changes every Tuesday and Friday, and publishing changes on Fridays.

Thanks!

@Shulammite-Aso
Copy link
Collaborator Author

This does not work for tag input however. Happens the input area for tags is created with a div having a class of "tokenfield" instead of the normal input like the other inputs and text areas
tag-area

I'll try another way to manipulate this div, as i'm not able to do it on the same place with the jquery multiple elements selector that i had used.

But if you have some pointers that can help @VladimirMikulic i'ld appreciate.

@Shulammite-Aso
Copy link
Collaborator Author

Also this pull request takes care of hiding footer when user starts typing on small.

For changing the look of the footer, i think i may need to do it on another pull request.
@VladimirMikulic that may be the pull request to solve this #449
what do you think?

@VladimirMikulic
Copy link
Contributor

You can style it in this PR as well since it is all related. Let's not create PR flood.

@Shulammite-Aso
Copy link
Collaborator Author

Okay👍

@Shulammite-Aso Shulammite-Aso mentioned this pull request Apr 12, 2020
5 tasks
Copy link
Collaborator

@NitinBhasneria NitinBhasneria left a comment

Choose a reason for hiding this comment

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

dist file should not have any changes. Thanks

@Shulammite-Aso
Copy link
Collaborator Author

@NitinBhasneria I did not manually make changes to the dist file. The changes resulted from my build.

@NitinBhasneria
Copy link
Collaborator

Before doing git add . do checkout the dist file by which the changes won't be added. Thanks

@Shulammite-Aso
Copy link
Collaborator Author

This now works for typing into tag input field as well
now-works-for--tags

@Shulammite-Aso
Copy link
Collaborator Author

Have a look @VladimirMikulic @jywarren

@VladimirMikulic
Copy link
Contributor

Please post a gif showing this on a real device. Thank you.

@Shulammite-Aso
Copy link
Collaborator Author

@VladimirMikulic how do i do that?

@VladimirMikulic
Copy link
Contributor

You can use a screen recording app.

@Shulammite-Aso
Copy link
Collaborator Author

I meant more of how i can get to visit the page on my phone

@VladimirMikulic
Copy link
Contributor

You can use a service like ngrok to forward port and make it accessible on your phone.

@Shulammite-Aso
Copy link
Collaborator Author

Okay. Thank's! 👍

Will continue with this tomorrow.

@Shulammite-Aso
Copy link
Collaborator Author

What i did to simplify the look of the footer on small screens was to show less message on the footer and let the user expand it when they need to, and also be able to shrink it back

ezgif com-video-to-gif

@Shulammite-Aso
Copy link
Collaborator Author

@VladimirMikulic please take a look.

@Shulammite-Aso
Copy link
Collaborator Author

Also please, I'm not always able to view my changes on the plots2 mock server. It takes days for them to begin to reflect, i don't understand why that is so.

Is there anything you know i can do about this??

@Shulammite-Aso
Copy link
Collaborator Author

@publiclab/reviewers Please review.☺

@Shulammite-Aso Shulammite-Aso marked this pull request as ready for review April 20, 2020 16:25
@VladimirMikulic
Copy link
Contributor

Your code could be refactored to promote reusability.
Both implementations have 99% code in common.

@Shulammite-Aso
Copy link
Collaborator Author

@VladimirMikulic can you tell me where you think could be refactored? Or comment on my code better still.

Copy link
Contributor

@VladimirMikulic VladimirMikulic left a comment

Choose a reason for hiding this comment

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

Hopefully, now it's clear.

@Shulammite-Aso
Copy link
Collaborator Author

Shulammite-Aso commented Apr 25, 2020

Now works for all situations. Please take a look, @VladimirMikulic
ple-footer

Copy link
Contributor

@VladimirMikulic VladimirMikulic left a comment

Choose a reason for hiding this comment

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

Please push the improvements.

src/core/Util.js Outdated Show resolved Hide resolved
src/PublicLab.Editor.js Outdated Show resolved Hide resolved
src/PublicLab.Editor.js Outdated Show resolved Hide resolved
src/modules/PublicLab.TagsModule.js Outdated Show resolved Hide resolved
@VladimirMikulic
Copy link
Contributor

Just to show you that JS is an unnecessary mess:

screen

Code:

.ple-footer .footer-block {
  display: inline;
}

@media (max-width: 600px) {
  .ple-footer .footer-block {
    display: flex;
    justify-content: space-between;
    align-items: center;
  }

  .ple-footer .footer-block #btn-history,
  .ple-footer .footer-block .ple-publish {
    padding: 0.5rem 1rem;
  }

  .ple-footer .footer-msg {
    font-size: 1.2rem;
  }
}
<div class="footer-block">
  <button id="btn-history" class="btn btn-lg btn-default btn-more">...</button>

  <span class="footer-msg">
    &nbsp; By publishing, you agree to
    <a href="https://publiclab.org/licenses">open source your work</a>
    <span class="hidden-xs"> so that others may use it.</span>
  </span>
</div>

<div class="footer-block">
  <button class="ple-publish btn btn-lg btn-primary pull-right disabled">
    Publish
  </button>

  <span class="ple-help pull-right">
    <span class="ple-steps-left">X</span>
    <span class="hidden-xs"> steps left</span>
  </span>
</div>

@Shulammite-Aso
Copy link
Collaborator Author

Just to show you that JS is an unnecessary mess:

screen

Code:

.ple-footer .footer-block {
  display: inline;
}

@media (max-width: 600px) {
  .ple-footer .footer-block {
    display: flex;
    justify-content: space-between;
    align-items: center;
  }

  .ple-footer .footer-block #btn-history,
  .ple-footer .footer-block .ple-publish {
    padding: 0.5rem 1rem;
  }

  .ple-footer .footer-msg {
    font-size: 1.2rem;
  }
}
<div class="footer-block">
  <button id="btn-history" class="btn btn-lg btn-default btn-more">...</button>

  <span class="footer-msg">
    &nbsp; By publishing, you agree to
    <a href="https://publiclab.org/licenses">open source your work</a>
    <span class="hidden-xs"> so that others may use it.</span>
  </span>
</div>

<div class="footer-block">
  <button class="ple-publish btn btn-lg btn-primary pull-right disabled">
    Publish
  </button>

  <span class="ple-help pull-right">
    <span class="ple-steps-left">X</span>
    <span class="hidden-xs"> steps left</span>
  </span>
</div>

Cool,
but I was also trying not to change the layout. I don't think some js hurts here, where we also get to preserve the original layout.

@VladimirMikulic
Copy link
Contributor

In my opinion, this looks nicer because the user is able to read the whole message + we avoid javascript which will likely become a debugging problem in the future. That's why CSS exists.

Let's see what others think.

@Shulammite-Aso
Copy link
Collaborator Author

In my opinion, this looks nicer because the user is able to read the whole message + we avoid javascript which will likely become a debugging problem in the future. That's why CSS exists.

Let's see what others think.

Okay.👍

@VladimirMikulic VladimirMikulic requested a review from a team April 27, 2020 19:48
@Shulammite-Aso
Copy link
Collaborator Author

Hi @VladimirMikulic , there hasn't been any comments from any other reviewer yet. If we can tell exactly how we want this to look. I can quickly make the changes.

@VladimirMikulic
Copy link
Contributor

Never mind, we can wait a few more days.

@jywarren jywarren changed the base branch from master to main June 24, 2020 15:09
@jywarren
Copy link
Member

Hi all, my apologies for not writing in sooner. Gosh, you've gone through a lot of work here. I know it can be challenging to go through so many iterations and appreciate both of your patience and understanding in going through the process and finding a workflow that works for both of you.

but I was also trying not to change the layout. I don't think some js hurts here, where we also get to preserve the original layout.

@Shulammite-Aso i think this is a good instinct because we need to be aware of how the corresponding HTML is structured in downstream usages, like in this file:

https://github.com/publiclab/plots2/blob/master/app/views/editor/rich.html.erb

Any CSS or JS we add here must be able to operate on the HTML used downstream; the examples/ files are just that - examples to guide usage. So if our suggested HTML changes, that could constitute a breaking change (thinking of https://semver.org) and we would have to make release notes to notify downstream projects that they must refactor their code to continue with the new version. When possible, we ought to avoid that. So, if there is a way that we can make good decisions here while avoiding breaking changes unless truly critical (security, or well-planned major version releases every few years, with plenty of warning), that's ideal. I hope this makes sense!

Thanks a lot, folks -- ❤️

@Shulammite-Aso
Copy link
Collaborator Author

Thank you for your input @jywarren

so currently, we have two changes in this PR; hiding the footer when typing on small screens and optimizing the look of the footer on the same smaller screens. The one we're not yet sure of whether the implementation is desirable or not is that of changing the look of the footer. If a change in this area is not quite necessary, we can remove it, and only merge that of hiding the footer when typing.

Or we can merge all our current changes (and maybe a few adjustments if needed) since it will mean no change to downstream html structure. here is what the footer looks like with using javascript to make the changes (sorry it's too zoommed out, i made the gif on a mobile phone) #464 (comment)

cc @cesswairimu @VladimirMikulic @Shreyaa-s @keshav234156 @NitinBhasneria please do leave your suggestion.

@jywarren
Copy link
Member

jywarren commented Jul 7, 2020

I guess my suggestion would be to use hidden-xs-style Bootstrap responsive breakpoints to hide elements with inline CSS classes, and that way we don't have JS to maintain or test, and also the styling we add would only affect the demo here and not installations of the editor in quite different HTML contexts.

That way we could use corresponding breakpoints to hide footer on plots2 as well! What do you think?

For Bootstrap 4 it's: https://getbootstrap.com/docs/4.4/utilities/display/

Bootstrap 3 is a bit simpler. plots2 is on Bootstrap 4 though!

Thank you!!!

@Shulammite-Aso
Copy link
Collaborator Author

Shulammite-Aso commented Jul 12, 2020

I guess my suggestion would be to use hidden-xs-style Bootstrap responsive breakpoints to hide elements with inline CSS classes, and that way we don't have JS to maintain or test, and also the styling we add would only affect the demo here and not installations of the editor in quite different HTML contexts.

That way we could use corresponding breakpoints to hide footer on plots2 as well! What do you think?

For Bootstrap 4 it's: https://getbootstrap.com/docs/4.4/utilities/display/

Bootstrap 3 is a bit simpler. plots2 is on Bootstrap 4 though!

wondering if we still need to upgrade to bootstrap 4, so we know if we need to consider upgrading it now and using bootstrap 4 classes in fixing both this issue and this #526, where we need to.
cc: @jywarren @NitinBhasneria @keshav234156 @shreyaa-sharmaa

@Shulammite-Aso
Copy link
Collaborator Author

Shulammite-Aso commented Jul 28, 2020

@jywarren i think we'll need focusin and focusout to hide the footer when typing, that's how JS may end up coming in here. unless maybe we want to hide the footer entirely on small screens.

@Shulammite-Aso
Copy link
Collaborator Author

I can remove footer design optimization from this PR, and do it in another PR if we still need to.

Thanks!!!

@jywarren
Copy link
Member

unless maybe we want to hide the footer entirely on small screens.

Yes, i think we can do this!

And, could we use both Bootstrap 3 and 4 classes to hide? That way we don't make a breaking change and remain compatible. Thank you!

@Shulammite-Aso
Copy link
Collaborator Author

Oh. The footer here is the PUBLISH bar actually. I don't think we can hide it. Folks we need to be able to publish there posts.

Sorry, I think i've not been descriptive enough with saying "hide ple-footer..." you may have thought i was referring to the main footer on publiclab website.

@Shulammite-Aso
Copy link
Collaborator Author

And i happen to have made this new PR #579 the same time you dropped your comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The publish bar should disappear making more space in small screen.
4 participants