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 #294: Better mobile experience #367

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

skw4y
Copy link

@skw4y skw4y commented Mar 17, 2019

Some Sonerezh theme hacks for a better mobile experience :

  • Navbar player on Bottom
  • Next/Previous icon modification
  • More space for the playing controls (play/next/previous)
  • Navbar collapse fix
  • Some controls are hidden on XS screens

Desktop Preview

desktop

Mobile Preview

mobile

@MightyCreak
Copy link
Contributor

That's promising! Could you add some screenshots for both web and mobile, please?

Copy link
Contributor

@MightyCreak MightyCreak left a comment

Choose a reason for hiding this comment

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

Some comments, after a quick review. Overall seems ok.

app/Config/bootstrap.php Outdated Show resolved Hide resolved
app/View/Layouts/default.ctp Show resolved Hide resolved
app/View/Layouts/default.ctp Outdated Show resolved Hide resolved
app/webroot/css/style.css Outdated Show resolved Hide resolved
app/webroot/js/navigation.js Outdated Show resolved Hide resolved
@skw4y
Copy link
Author

skw4y commented Mar 17, 2019

I've made some changes since the creation of the repository but you can preview some screenshots there:
https://github.com/skw4y/sonerezh-mobile

I didn't find the existing screenshots of the project but sure I can add some.

Copy link
Contributor

@MightyCreak MightyCreak left a comment

Choose a reason for hiding this comment

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

Some tab issues

app/View/Layouts/default.ctp Outdated Show resolved Hide resolved
app/View/Layouts/default.ctp Outdated Show resolved Hide resolved
app/View/Layouts/default.ctp Outdated Show resolved Hide resolved
app/webroot/css/style.css Outdated Show resolved Hide resolved
Copy link
Author

@skw4y skw4y left a comment

Choose a reason for hiding this comment

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

Tabs replaced by spaces

@MightyCreak
Copy link
Contributor

MightyCreak commented Mar 18, 2019

@lGuillaume124 For me it seems all good. I actually prefer the player on the bottom since it follows the industry standard and definitely makes more sense on mobile.

@skw4y From the mobile screenshot, I don't see these buttons:
image

Is it possible to access them somehow, or they're completely hidden when on mobile?

@skw4y
Copy link
Author

skw4y commented Mar 18, 2019

@MightyCreak On mobile the shuffle button is visible but the other two are completely hidden for the moment:
sonerezh_mobile

@MightyCreak
Copy link
Contributor

The screenshots on your changes seem a bit outdated (for instance the play controls are now on the bottom right, it is on the bottom left in your screenshots). Could you just refresh the ones from your changes? The interface here on master didn't change that much (though a discussion is in progress).

@skw4y
Copy link
Author

skw4y commented Mar 18, 2019

Yes sure, I was pointing the one in my repository just to give you an idea but I'll refresh them tonight.

@MightyCreak
Copy link
Contributor

Thank you! Much appreciated 😉

@skw4y
Copy link
Author

skw4y commented Mar 18, 2019

I've just updated the screenshots on my sonerezh-mobile repo but I can also add them to the current pull request if you want.

@MightyCreak
Copy link
Contributor

Thanks!

If you could add them in the PR description, that would be awesome (the more descriptive a PR is, the more probable it will be merged, because it's easier to get in and review since we know what to expect).

But the final word will be for @lGuillaume124 anyway 😉

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.

2 participants