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

Sunder gutka toolbar #532

Merged
merged 35 commits into from
Mar 21, 2019

Conversation

inderpreetsingh
Copy link
Collaborator

@inderpreetsingh inderpreetsingh commented Mar 12, 2019

fixes #354 #433 #437

  • Add sunder gutka UI presenter mode
  • Add sunder gutka UI non-presenter mode
  • Add realm support
  • Add sqlite support
  • Adds close button
  • Clean/optimize code
  • talk and figure out about special verses / lines with different db structure.

@inderpreetsingh inderpreetsingh marked this pull request as ready for review March 17, 2019 12:22
Copy link
Contributor

@tsingh777 tsingh777 left a comment

Choose a reason for hiding this comment

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

Please add .DS_Store to gitignore

www/js/toolbar.js Outdated Show resolved Hide resolved
www/js/search.js Outdated Show resolved Hide resolved
www/js/toolbar.js Show resolved Hide resolved
@tsingh777
Copy link
Contributor

Looks really good!
Some observations:

  • long banis have severely degraded the performance of the spacebar shortcut. it locks up the UI. We have to fix that before releasing.
  • when in non presenter view and the window is collapsed, it looks like: image probably needs some padding or a smaller icon?
  • some lines just end up being blank:
    image
  • nitnem banis probably don't need to be in popular banis (arti arta)
    image
  • at 1040px-1050px width it looks like this
    image
  • The what's new modal is now showing up behind the other windows:
    image
  • should sundar gutka populate history? It isn't with these changes c c.@maneetpaul
  • is there a duplicate line in the sundar gutka anand sahib?
    image
  • is Anand Sabhib & Salok a good enough differentiator for 6 Pauris vs 40 Pauris? cc. @maneetpaul

  • Feature request: can we have translit somewhere in the bani list?
  • Feature request: with long banis, it going to become difficult scrolling to find where in the bani the user needs to present. should we add a search or something like the bookmark in sundar gutka that takes us a specific place in the bani.

@maneetpaul
Copy link
Contributor

For the Nitmem section, please have:

-- Japjee Sahib
-- Jaap Sahib
-- Tav Prasaad Svwaiye
-- Benti Chupai Sahib
-- Anand Sahib
-- Rehras Sahib
-- Sohila Sahib

For popular, please use:

-- Asa Kee Vaar
-- Aarti
-- Salok M. 9
-- Sukhmani Sahib

@tsingh777 responses to some of your notes:

  • For now, leave sundar gutka out of history. We will revisit that in a future Sprint along with a few other fixes/updates
  • Yes, I believe we differentiate the two Anand Sahibs well with the current wording
  • If it is an easy add, yes, I agree we should add a toggle to the bani list to show it in english vs gurmukhi. If this will take a lot more time, we will save it for future Sprint. @inderpreetsingh please let us know.
  • I think people will use normal search when they are looking for a specific line inside of a ban (rather than open the entire bani and scroll for it). We should see what kind of feedback we get. Will keep an eye on this. Additionally, people can type the first letter of the line they are looking for once the bani is open and skip through pretty quickly like that (this was an old STTM2 feature).

Thank you for the comments!

@inderpreetsingh
Copy link
Collaborator Author

inderpreetsingh commented Mar 19, 2019

Fixed everything except DB related bugs or new features. 🙏

I wasn't able to reproduce locking up of UI @tsingh777 can you let me know which bani caused that to happen? Also, what do you suggest as a solution for increasing performance? Should we not load the whole bani at once (and use pagination) @maneetpaul ?

Using translit I get these spellings, do we want to go ahead with these spellings?
and we don't have English names of banis in the database. We have tokens but they don't have spaces in bani names.

I have added and commented the code.

Screenshot 2019-03-19 at 2 16 15 PM

@inderpreetsingh
Copy link
Collaborator Author

I say. 👍 +1 to adding bookmarks. I think we have them in database too. 🙏

@maneetpaul
Copy link
Contributor

I am not a fan of the current English list. Some of the spellings look really strange and we need to make each bani fit on one line so it looks cleaner. Let's release 5.0 with Gurmukhi only for now, and we'll add the English names for each Bani to the DB properly. We'll add a toggle in a future update to switch between the two.

Regarding pagination, I am against requiring the user needing to do anything extra in order to go through the entire bani (they should be able to go through the whole thing by pressing the down arrow). Let's try to isolate the issue and see if it is specific to a certain bani.

Copy link
Contributor

@maneetpaul maneetpaul left a comment

Choose a reason for hiding this comment

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

I just noticed this, but it should be spelled "Sundar Gutka" with an "a" in "Sundar".

www/index.html Outdated Show resolved Hide resolved
www/js/toolbar.js Outdated Show resolved Hide resolved
www/js/toolbar.js Outdated Show resolved Hide resolved
@tsingh777
Copy link
Contributor

Sukhmani Sahib is the least performant.

The following function from index.js needs to be optimized. I will look into it.

function spaceBar(e) {
  const mainLineID = search.$shabad.querySelector('a.panktee.main').dataset
    .lineId;
  const currentLineId = search.$shabad.querySelector('a.panktee.current')
    .dataset.lineId;

  let newLineId = mainLineID;

  if (mainLineID === currentLineId) {
    let done = false;
    search.$shabad.querySelectorAll('a.panktee').forEach((item) => { //2099 lines for Sukhmani sahib
      if (!item.classList.contains(['seen_check']) && !done) {
        newLineId = item.dataset.lineId;
        done = true;
      }
    });
  }

  highlightLine(newLineId);
  e.preventDefault();
}

maybe we can look into adding virtual scrolling as well. not sure if a libaray exists that can help here. cc. @navdeepsinghkhalsa @bogas04

@tsingh777
Copy link
Contributor

Copy link
Contributor

@tsingh777 tsingh777 left a comment

Choose a reason for hiding this comment

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

please remove www/.DS_Store and add to git ignore

www/js/defaults.json Outdated Show resolved Hide resolved
www/js/viewer.js Outdated Show resolved Hide resolved
www/js/toolbar.js Show resolved Hide resolved
@@ -35,6 +35,9 @@
"autoplay-options": {
"autoplayTimer": 10
},
"sunder-gutka": {
"bani-length": 4
Copy link
Contributor

Choose a reason for hiding this comment

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

let's use the same default as the sundar gutka app? I think that one is 2 or 3. cc @maneetpaul @tarunsingh5

Copy link
Contributor

Choose a reason for hiding this comment

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

can be followed up in separate pr

Copy link
Contributor

Choose a reason for hiding this comment

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

The new SG update requires users to choose a length upon the first app launch. Prior to that, the default was "Very Long".

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we can implement something similar and make it a choose on first install

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's see what the UX is like during Alpha testing and we'll go from there. Bani length only impacts ਬੇਨਤੀ ਚੌਪਈ ਸਾਹਿਬ, ਰਹਰਾਸਿ ਸਾਹਿਬ, ਆਰਤੀ, and ਸੋਹਿਲਾ ਸਾਹਿਬ

tsingh777
tsingh777 previously approved these changes Mar 20, 2019
@@ -20,8 +21,9 @@ function hideSlide() {
global.controller.sendText('');
}

function highlightLine(newLine) {
const $line = search.$shabad.querySelector(`#line${newLine}`);
function highlightLine(newLine, nextLineCount = null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: the variables names (nextLineCount) here are bit confusing.

@maneetpaul maneetpaul merged commit c288d31 into KhalisFoundation:dev Mar 21, 2019
@inderpreetsingh inderpreetsingh deleted the sunderGutkaToolbar branch May 11, 2020 01:22
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.

Surface Sundar Gutka
3 participants