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

[4.0] Skip-to plugin major upgrade [a11y] #31854

Closed
wants to merge 5 commits into from

Conversation

brianteeman
Copy link
Contributor

This is a draft PR to give a heads up about this new version especially as there are new language strings.

Amongst other things it addresses @infograf768 comments about not everything be translatable and an old comment from @clodder about the dropdown script which is no longer being used.

In the process of creating this PR I discovered a few bugs in the upstream code (pr submitted) and that there github is ahead of the release on npm yet with a lower package number.

As a result this PR is not completed and its not working (hence its a draft).

This release is much much better under the hood. I have only implemented some of the features (ie not the important and all distinction) as they're more suitable to a content rich site than an admin ui.

For now you can only do a code review. As soon as I get a response to https://github.com/paypal/skipto/issues/68 I will be able to complete the release

More information http://paypal.github.io/skipto/

This is a draft PR to give a heads up about this new version especially as there are new language strings.

Amongst other things it addresses @infograf768 comments about not everything be translatable and an old comment from @clodder about the dropdown script which is no longer being used.

In the process of creating this PR I discovered a few bugs in the upstream code (pr submitted) and that there github is ahead of the release on npm yet with a lower package number.

As a result this PR is not completed and its not working (hence its a draft).

This release is much much better under the hood. I have only implemented some of the features (ie not the important and all distinction) as they're more suitable to a content rich site than an admin ui.

For now you can only do a code review. As soon as I get a response to https://github.com/paypal/skipto/issues/68 I will be able to complete the release
@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.0-dev labels Jan 4, 2021
@@ -52,24 +52,46 @@ public function onAfterDispatch()
return;
}

// Are we in a modal?
$input = Factory::getApplication()->input;
if ($input->get('tmpl', '', 'cmd') === 'component')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if ($input->get('tmpl', '', 'cmd') === 'component')
if ($input->get('tmpl', '', 'cmd') === 'component')

},
"css": {
"src/css/SkipTo.css": "css/SkipTo.css"
"src/css/skipTo.css": "css/skipTo.css"
Copy link
Member

Choose a reason for hiding this comment

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

if the case is changed we need #31804 first and add the corresponding files to the rename function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is still up in the air until upstream fixes their npm etc.

They do the css differently to the way we did it so this line will probably be deleted anyway

Copy link
Member

Choose a reason for hiding this comment

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

OK but the changed the .js file too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes I know. I cant do anything about it until they fix their end

@dgrammatiko
Copy link
Contributor

@brianteeman fwiw if paypal/skipto#69 is ever gets merged then the whole inline part of the plugin is useless (not to mention that the option type="module" is not doing what you think it does, in sort the plugin will NEVER execute in browsers without module support)

@brianteeman
Copy link
Contributor Author

@dgrammatiko thanks for #69 that will really make it much better

As for the module bit, I don't actually have a clue what it does. @Fedik wrote that

@brianteeman
Copy link
Contributor Author

Good news is that I have it working locally

Bad news is that they made even more breaking changes and they put them in a patch release. So as soon as that is fixed and they release 4.0 I will be ready

@brianteeman
Copy link
Contributor Author

closed see #32043

@brianteeman brianteeman deleted the skipto branch January 15, 2021 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Language Change This is for Translators NPM Resource Changed This Pull Request can't be tested by Patchtester
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants