-
-
Notifications
You must be signed in to change notification settings - Fork 135
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
Reopen last article #1090
Reopen last article #1090
Conversation
OK, great. So I can review it, can you please update the branch again (button below), do some manual tests in both jQuery and ServiceWorker modes, and then let me know when it is ready for review. |
Please also link this PR to the issue that it closes. |
@Rbcoder1 You will see that this test is failing: https://github.com/kiwix/kiwix-js/actions/runs/6039794791/job/16393978914?pr=1090. The reason is given there. However, you should not be adding a require script to this Repo. We spent a long time removing require.js and converting the repository to ES6 modules. You should only use the ES6 module syntax. However, I can't really understand why you've included this file, because you don't seem to use any require statements in the rest of your code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
every chaged
@Rbcoder1 I've done some quick tests, and the general functionality is working, which is great. However, I noticed the following issues:
|
NB I haven't reviewed your code yet, but in general terms I notice you have blocks of repeated code. It would be much better if you were to consolidate required code in a function, and re-use the What I suggest you do is make a separate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see comments above for requested changes.
Is this PR still alive? |
Yes, it is. I Work On It.
Just Break Due To Exam Now I Am Live On It.
…On Wed, 18 Oct 2023, 3:13 pm Jaifroid, ***@***.***> wrote:
Is this PR still alive?
—
Reply to this email directly, view it on GitHub
<#1090 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AZRVEQWIW4KXZQCX2OQ6N2TX76QFJAVCNFSM6AAAAAA4GJ7YJKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONRYGA3TOMJQHE>
.
You are receiving this because you were assigned.Message ID:
***@***.***>
|
OK, no problem. No rush, I just wanted to check. |
Why My Test Are Failed. |
Hi, I'm just running them to see. |
Tests are passing now. |
I done with all problem can You check it again is their any changes need |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see the stylistic changes, and also one exception produced by your code in console. Also, you need to attend to accessibility, which means you need to ensure your implementation can be used by keyboard-only users, using tab key and spacebar, not mouse. The blur()
method you use is preventing keyboard use of the UI.
Ok solve them
…On Sun, 12 Nov 2023, 2:23 pm Jaifroid, ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Please see the stylistic changes, and also one exception produced by your
code in console. Also, you need to attend to accessibility, which means you
need to ensure your implementation can be used by keyboard-only users,
using tab key and spacebar, not mouse. The blur() method you use is
preventing keyboard use of the UI.
------------------------------
In www/js/app.js
<#1090 (comment)>:
> @@ -379,6 +379,7 @@ document.getElementById('btnTop').addEventListener('click', function (event) {
document.getElementById('btnHome').addEventListener('click', function (event) {
// Highlight the selected section in the navbar
event.preventDefault();
+ this.blur();
Is it necessary to do this? It prevents keyboard users from keeping focus
on the button, and so prevents them toggling with the space key. This
reduces accessibility.
------------------------------
In www/js/app.js
<#1090 (comment)>:
> @@ -408,13 +409,20 @@ document.getElementById('btnHome').addEventListener('click', function (event) {
});
document.getElementById('btnConfigure').addEventListener('click', function (event) {
event.preventDefault();
+ this.blur();
Ditto
------------------------------
In www/js/app.js
<#1090 (comment)>:
> @@ -423,13 +431,21 @@ document.getElementById('btnConfigure').addEventListener('click', function (even
});
document.getElementById('btnAbout').addEventListener('click', function (event) {
event.preventDefault();
+ this.blur();
Ditto
------------------------------
In www/js/app.js
<#1090 (comment)>:
> // Highlight the selected section in the navbar
document.getElementById('liHomeNav').setAttribute('class', '');
document.getElementById('liConfigureNav').setAttribute('class', '');
document.getElementById('liAboutNav').setAttribute('class', 'active');
$('.navbar-collapse').collapse('hide');
- // Show the selected content in the page
- uiUtil.tabTransitionToSection('about', params.showUIAnimations);
+
Remove redundant space
------------------------------
In www/js/lib/uiUtil.js
<#1090 (comment)>:
> @@ -862,15 +862,12 @@ function applyAppTheme (theme) {
// Displays the return link and handles click event. Called by applyAppTheme()
function showReturnLink () {
- var viewArticle = document.getElementById('viewArticle');
Since you have more than one document.getElementById('viewArticle')
statements below, it would be much better to define a variable here (like
it used to do). Obviously you need t give it a different name, like
viewArticleLink, to avoid a clash with the viewArticle() function.
Alternatively, you could rename the viewArticle function to showArticle().
It's up to you, whatever is easiest.
------------------------------
In www/js/lib/uiUtil.js
<#1090 (comment)>:
> e.preventDefault();
document.getElementById('liConfigureNav').classList.remove('active');
document.getElementById('liHomeNav').classList.add('active');
- tabTransitionToSection('home', params.showUIAnimations);
- const welcomeText = document.getElementById('welcomeText');
- welcomeText.style.display = 'none';
+ viewArticle();
viewArticle.style.display = 'none';
In your current code, this line is causing an error (see below). Because
viewArticle is a function, not a DOM element.
[image: image]
<https://user-images.githubusercontent.com/4304337/282288051-2b1eb841-856e-4608-82ea-ce38c174eeee.png>
------------------------------
In www/js/app.js
<#1090 (comment)>:
> // Highlight the selected section in the navbar
document.getElementById('liHomeNav').setAttribute('class', '');
document.getElementById('liConfigureNav').setAttribute('class', 'active');
document.getElementById('liAboutNav').setAttribute('class', '');
$('.navbar-collapse').collapse('hide');
- // Show the selected content in the page
- uiUtil.tabTransitionToSection('config', params.showUIAnimations);
+ if (document.getElementById('configuration').style.display == 'none') {
+ // Show the selected content in the page
+ uiUtil.tabTransitionToSection('config', params.showUIAnimations);
+ }
+ else{
Please use the standard style rules as required by ESLint. This should be }
else { (on one line).
------------------------------
In www/js/app.js
<#1090 (comment)>:
> // Highlight the selected section in the navbar
document.getElementById('liHomeNav').setAttribute('class', '');
document.getElementById('liConfigureNav').setAttribute('class', '');
document.getElementById('liAboutNav').setAttribute('class', 'active');
$('.navbar-collapse').collapse('hide');
- // Show the selected content in the page
- uiUtil.tabTransitionToSection('about', params.showUIAnimations);
+
+ if (document.getElementById('about').style.display == 'none') {
+ // Show the selected content in the page
+ uiUtil.tabTransitionToSection('about', params.showUIAnimations);
+ }
+ else{
Ditto about } else {.
—
Reply to this email directly, view it on GitHub
<#1090 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AZRVEQVRORTE65CBNULTG3LYECE7FAVCNFSM6AAAAAA4GJ7YJKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOMRWGIYDANJTGU>
.
You are receiving this because you were assigned.Message ID:
***@***.***>
|
Just a quick suggestion: compare your implementation with the online version at https://browser-extension.kiwix.org/ (let it update to v3.11.0). You'll see there that when a user clicks on one of the tabs, it remains "pressed" until they go to a different tab. I think this is quite important for an "unclick" solution like yours. Maybe when you remove the "blur()" method from your code, this will just work as it is expected to. |
Ok
I do it.
…On Mon, 13 Nov 2023, 2:43 am Jaifroid, ***@***.***> wrote:
Just a quick suggestion: compare your implementation with the online
version at https://browser-extension.kiwix.org/ (let it update to
v3.11.0). You'll see there that when a user clicks on one of the tabs, it
remains "pressed" until they go to a different tab. I think this is quite
important for an "unclick" solution like yours. Maybe when you remove the
"blur()" method from your code, this will just work as it is expected to.
—
Reply to this email directly, view it on GitHub
<#1090 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AZRVEQWNMTCG4XNSUKKYTKLYEE3YFAVCNFSM6AAAAAA4GJ7YJKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBXGI2DCMRXG4>
.
You are receiving this because you were assigned.Message ID:
***@***.***>
|
00b3235
to
5794f56
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all changes are reviewed and updated please check it again
OK, thanks, I'll check again. |
@Rbcoder1 I just tested your PR again, and I immediately noticed this regression. I've mentioned this problem a number of times above. Please thoroughly test your PR , extensively, before asking for review, to make sure all such issues are ironed out. Please see https://github.com/kiwix/kiwix-js/blob/main/CONTRIBUTING.md#testing for all the testing you should do, and which should have picked this up straight away. |
I Have Successfully Implemented The Functionality of Reopening last article after unclicking cofiguration and about button.