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

Revoke Navigation Bar UI #996

Closed
wants to merge 30 commits into from
Closed

Revoke Navigation Bar UI #996

wants to merge 30 commits into from

Conversation

Rbcoder1
Copy link

Changed The UI Of Navigation Bar and Revoke in as per PWA

As Per Our Discussion Here:- https://github.com/kiwix/kiwix-js/issues/523#issuecomment-1510203437 on this issue. I changed the navigation bar UI like PWA and Make a pull request

In this pull request I changed are as follow :

  • Changed the Kiwix Text into Kiwix Icon.
  • Move Search Bar In Top Menu
  • And Also Try To Re-engineer the collapsing of hamburger in very small screen.

KIwix Icon Chaged
I changed the kiwix text into kiwix icon that used in pwa version of kiwix

Serch Bar
Move the search bar in top navbar session as per pwa version

changed other text into icons
changes all text nav link into icon (about,config,random,search)

re-engine the hamburger
Change the hamburger into Three dots and make it visible in verticle or column view.

If Any Problem or Bug In request I Will Solved ASAP
That changes are made in first pull request and the remain are changed in next pull request.

Kiwix-JS Before Changes

kiwix before

Kiwix-JS After Changes
kiwix after

Hamburger :

Before Changes :

res2
res4

After Changes :

res1
res3

@Jaifroid
Copy link
Member

Thanks for the new PR. However, the usual practice is that you simply push your changes to the same PR, rather than closing one that has errors and then opening a new one. No problem this time, but if changes are needed to this PR, simply push them to this one. Many thanks.

@Jaifroid
Copy link
Member

PS I'll test and review when I get a chance. It's passing automated tests.

@Rbcoder1
Copy link
Author

Ok

@Jaifroid
Copy link
Member

Jaifroid commented Apr 29, 2023

Thank you very much for this PR. It's a good start, and you have an interesting concept for the kebab menu!

Some suggestions:

  • I think the search button should always be showing, and always next to the search field.. Can you exclude it from the kebab?
  • The Kiwix icon (top home) is always pressed after you load an archive and are browsing a ZIM, which looks a bit odd. I suggest it should be "unpressed" after a short period.
  • Clicking on some of the buttons (e.g. Config) doesn't work unless you centre the mouse directly over the icon inside the button. You should make the button clickable, not the icon within it;
  • Like in Kiwix JS PWA, buttons should be "unclickable" (if I am on Home, and I click Config, then I should be able to click Config again and it will return me to Home).

I noticed some rough edges:

  • Once the kebab button is pressed, resizing the window (making it larger) doesn't close the vertical menu;
  • The buttons in the kebab are unresponsive for me;
  • Spacing and positioning of icons isn't always correct (see screenshots below) -- additionally, the text in the search box should be further to the left, so as not to waste space.

I think the kebab should only show at very narrow screen widths. Mostly, there is enough space at normal mobile screen widths. And perhaps you can consider always showing the Config icon (as well as the search icon), so that only the Random and About buttons get hidden in the kebab? That might not work on the narrowest screens, I guess, but it's worth experimenting.

NB I haven't reviewed the code.

image
image

@Rbcoder1
Copy link
Author

Ok I will Fix this fix Push It.
Thanks for give this best suggestions.

@Rbcoder1
Copy link
Author

Rbcoder1 commented May 3, 2023

Thank you very much for this PR. It's a good start, and you have an interesting concept for the kebab menu!

Some suggestions:

  • I think the search button should always be showing, and always next to the search field.. Can you exclude it from the kebab?
  • The Kiwix icon (top home) is always pressed after you load an archive and are browsing a ZIM, which looks a bit odd. I suggest it should be "unpressed" after a short period.
  • Clicking on some of the buttons (e.g. Config) doesn't work unless you centre the mouse directly over the icon inside the button. You should make the button clickable, not the icon within it;
  • Like in Kiwix JS PWA, buttons should be "unclickable" (if I am on Home, and I click Config, then I should be able to click Config again and it will return me to Home).

I noticed some rough edges:

  • Once the kebab button is pressed, resizing the window (making it larger) doesn't close the vertical menu;
  • The buttons in the kebab are unresponsive for me;
  • Spacing and positioning of icons isn't always correct (see screenshots below) -- additionally, the text in the search box should be further to the left, so as not to waste space.

I think the kebab should only show at very narrow screen widths. Mostly, there is enough space at normal mobile screen widths. And perhaps you can consider always showing the Config icon (as well as the search icon), so that only the Random and About buttons get hidden in the kebab? That might not work on the narrowest screens, I guess, but it's worth experimenting.

NB I haven't reviewed the code.

I Update The Pull Request Yet. And Change The Code And Fix Issue you Pointed Above

  • Fix the Search Button Problem And Exclude it From Kebab Menu and Fix the spacing problem.
  • Fix the top-home Icon always pressed problem and it will unpressed after 3 seconds.
  • Fix the problem of button click in center now you can click anywhere on entire button it will affect.
  • Fix the problem of vertical menu is not closing after resizing window now it will working better you can resize window anywhere.
  • Fix the problem related to search spacing remove the extra space and code again to ensuring that no wasting of space.

You can Check the Fix and Test that it will help or not.

But I am not understanding this :

  • The buttons in the kebab are unresponsive for me;

PRimg1
PRimg2

@Rbcoder1
Copy link
Author

Rbcoder1 commented May 3, 2023

I Have One Idea That In Kiwix-Js We Remove The Text Version and Replace it To Kiwix Icon so We can not easily view the current version of kiwix now. yes it will be on another place in app. but i think when we hover on the top-icon we show the current version of kiwix-js there.

@Jaifroid
Copy link
Member

Jaifroid commented May 3, 2023

I Have One Idea That In Kiwix-Js We Remove The Text Version and Replace it To Kiwix Icon so We can not easily view the current version of kiwix now. yes it will be on another place in app. but i think when we hover on the top-icon we show the current version of kiwix-js there.

I think hovering over the icon to view the version is OK, but remember it won't work on mobile.. So we should have a text version somewhere. See where I put it on the PWA (pwa.kiwix.org). This could be a separate issue/PR, as you wish.

@Jaifroid
Copy link
Member

Jaifroid commented May 3, 2023

  • The buttons in the kebab are unresponsive for me

When I click on the kebab and then on one of the buttons that it show, nothing happens. But I'll test that again with your new commits and let you know if it still persists.

@Rbcoder1
Copy link
Author

Rbcoder1 commented May 3, 2023

  • The buttons in the kebab are unresponsive for me

When I click on the kebab and then on one of the buttons that it show, nothing happens. But I'll test that again with your new commits and let you know if it still persists.

Yes it will fix in this commit

@Rbcoder1
Copy link
Author

Rbcoder1 commented May 3, 2023

I Have One Idea That In Kiwix-Js We Remove The Text Version and Replace it To Kiwix Icon so We can not easily view the current version of kiwix now. yes it will be on another place in app. but i think when we hover on the top-icon we show the current version of kiwix-js there.

I think hovering over the icon to view the version is OK, but remember it won't work on mobile.. So we should have a text version somewhere. See where I put it on the PWA (pwa.kiwix.org). This could be a separate issue/PR, as you wish.

Yes
Else we can can add it into about / Config tab

@Rbcoder1
Copy link
Author

Rbcoder1 commented May 7, 2023

I am really sorry Understand that it was formated by my editor. That's why it look like this
I revert many files from them and there are only 5 files that I changed so revert back all unnecessary changes

@Jaifroid
Copy link
Member

Jaifroid commented May 7, 2023

I am really sorry Understand that it was formated by my editor. That's why it look like this I revert many files from them and there are only 5 files that I changed so revert back all unnecessary changes

There's no need to apologize! I hope it's not too complicated to fix. You should find the setting in your editor and make sure you prettify (if you want to) only the sections of code that you have written, or that you are currently working on. You can usually just select the code you want to prettify, taking care to select only code you've written, or code that needs to be changed to accommodate what you've written.

@Rbcoder1
Copy link
Author

Rbcoder1 commented May 7, 2023 via email

Copy link
Author

@Rbcoder1 Rbcoder1 left a comment

Choose a reason for hiding this comment

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

I Review This Files And Revert Back uneccessary Changes From That

Rbcoder1

This comment was marked as duplicate.

Copy link
Author

@Rbcoder1 Rbcoder1 left a comment

Choose a reason for hiding this comment

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

All changes reverted please check them again and tell is it right or any other changes needed.

Rbcoder1

This comment was marked as duplicate.

Rbcoder1

This comment was marked as duplicate.

@Rbcoder1
Copy link
Author

@Jaifroid Please Check It Again .
AND tell Me Is their Any changes Needed

var btn = document.getElementById("collapbtn");
var navIcons = document.getElementById("horizontalNavBtn");

btn.addEventListener('click', () => {
Copy link
Member

Choose a reason for hiding this comment

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

As mentioned in CONTRIBUTING.md, we can't use arrow function syntax in this Repo yet, because we are still supporting old browsers and we are not yet using Babel to transpile. Please change.

Copy link
Member

@Jaifroid Jaifroid left a comment

Choose a reason for hiding this comment

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

Apologies for the slow review, I've been completely snowed under with the day job.

Thank you for removing most of the extraneous code. There is still some small stuff to be removed (see comments).

It's good work so far! But I noticed some problems:

  • The about button sometimes disappears when resizing but the hamburger menu doesn't always show instead;
  • The Home button (Kiwix icon) remains "pressed", and doesn't seem to go back to neutral after a few seconds (you had mentioned this functionality);
  • If I click the Config button, I can't then "unclick" it to go back to the currently loaded article.
  • If I'm in Configuration and click the Random button, it doesn't then display the article;
  • The vertical menu sometimes gets misaligned, which looks quite ugly (see screenshot);
  • I wonder if it would be better for the hamburger menu to show a second row of buttons rather than stack them vertically. The vertical layout option is not very standard.

image

@@ -71,12 +71,13 @@ define(['jquery', 'zimArchiveLoader', 'uiUtil', 'settingsStore','abstractFilesys
* WARNING: Only change these paramaeters if you know what you are doing
*/
// The current version number of this app
params['appVersion'] = '3.8.1'; // **IMPORTANT** Ensure this is the same as the version number in service-worker.js
params['appVersion'] = '3.8.1' ; // **IMPORTANT** Ensure this is the same as the version number in service-worker.js
Copy link
Member

Choose a reason for hiding this comment

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

Please remove space, it's not part of this PR.

// The PWA server (currently only for use with the Mozilla extension)
params['PWAServer'] = 'https://moz-extension.kiwix.org/current/'; // Include final slash!
// params['PWAServer'] = 'https://kiwix.github.io/kiwix-js/'; // DEV: Uncomment this line for testing code on GitHub Pages
// params['PWAServer'] = 'http://localhost:8080/'; // DEV: Uncomment this line (and adjust) for local testing
// A parameter to determine the Settings Store API in use

Copy link
Member

Choose a reason for hiding this comment

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

Remove redundant extra line.

Copy link
Member

Choose a reason for hiding this comment

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

This file should not be altered in any way, as it's a dependency.

Copy link
Member

Choose a reason for hiding this comment

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

This file should not be altered in any way, as it is not part of the PR.

@Jaifroid
Copy link
Member

Additionally, it's not possible to "unclick" the About button. See the PWA version for what I mean. When you have an article loaded, click on the Config icon, then click on that icon again (this is what I mean by "unclick"). Also do the same with the About button.

@Rbcoder1
Copy link
Author

Rbcoder1 commented May 21, 2023 via email

@Rbcoder1
Copy link
Author

I want to rework on this issue

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.

2 participants