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

J4admin updated webcomponents #132

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

Conversation

sahidhossen
Copy link

No description provided.

@wilsonge
Copy link
Contributor

wilsonge commented Nov 4, 2019

Variables are superficially there. Updated the key and restored it in travis and have updated the config to the latest travis approved version in their docs. works in the master branch now. So update your branch to master here and see what happens I guess.

Also can we remove the yarn-error log file from here please

.gitignore Outdated Show resolved Hide resolved
@wilsonge
Copy link
Contributor

wilsonge commented Nov 4, 2019

Just fixed the failures in the build quickly so hopefully we should get green when the branch is updated :)

docs/callout.md Outdated
Message body is optional. If help documentation is available, consider adding a link to learn more
</div>
<div class="callout-footer">
<a href="#" class="callout-link" target="blank">Learn more</a>
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldnt this and all similar links be target="_blank"

Copy link
Author

Choose a reason for hiding this comment

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

Okey we will check it out.

@wilsonge
Copy link
Contributor

wilsonge commented Nov 4, 2019

I don't get the failure :( https://travis-ci.org/joomla-projects/custom-elements/builds/607091608#L445 we can see the username is there. Obviously the access key is encrypted so not showing - but it has the same all branches config as the username - and we can see in the master branch it's clearly working.

@sahidhossen
Copy link
Author

@wilsonge Is this the reason for the failure?

Encrypted environment variables have been removed for security reasons. See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions

@wilsonge
Copy link
Contributor

wilsonge commented Nov 4, 2019

Ahh yeah that would do it. They even mention sauce labs as an explicit example there 🤔 . So with travis we can't actually run the sauce lab tests until we've merged the pull request unless you've got write access to the project. Sounds fairly broken :)

@brianteeman
Copy link
Contributor

brianteeman commented Nov 4, 2019

The following code is not correct because it may result in invalid markup
const accordionTitle = document.createElement('h3');

By forcing the title to be a h3 then you can get an h3 directly after an h1 which is a serious accessibility failure

There may be other instances of this error - I just highlighted one as an example

@sahidhossen
Copy link
Author

@brianteeman we create h3 tag inside joomla-accordion component and each accordion content always inside a section tag.

But if its a serious accessibility issue then can you suggest alternative solution for this?

For accordion I think we can use div rather than h3 .

@brianteeman
Copy link
Contributor

Yes its a serious accessibility issue so please make it a div AND also check that there are no other places that an Hx is used

@sahidhossen
Copy link
Author

Ok I will update in accordion and check other places regarding the issue.
Thanks for your suggestion.

@C-Lodder
Copy link
Contributor

C-Lodder commented Nov 5, 2019

The days when H tags were used based on sizing lol

@sahidhossen
Copy link
Author

Hi @C-Lodder ,
If it has no other issue then can you please merge this pull request and run CI ?

@C-Lodder
Copy link
Contributor

I don't have the permissions to merge


joomla-callout::before {
content: "";
width: 0px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
width: 0px;
width: 0;

joomla-callout::before {
content: "";
width: 0px;
height: 0px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
height: 0px;
height: 0;

.accordion-panel-title:hover {
background-color: #edf4fa; }
.accordion-panel-title:focus {
box-shadow: 0px 0px 0px 2px #198df8;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
box-shadow: 0px 0px 0px 2px #198df8;
box-shadow: 0 0 0 2px #198df8;

joomla-accordion.accordion .accordion-panel-title:hover {
background-color: #edf4fa; }
joomla-accordion.accordion .accordion-panel-title:focus {
box-shadow: 0px 0px 0px 2px #198df8;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
box-shadow: 0px 0px 0px 2px #198df8;
box-shadow: 0 0 0 2px #198df8;

joomla-pagination .minimize-items-wrapper ul .pagination-item > a {
padding: 1px 5px;
border-bottom: 1px solid var(--bg-color);
border-radius: 0px; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
border-radius: 0px; }
border-radius: 0; }

joomla-accordion.accordion .accordion-panel-title:hover {
background-color: #edf4fa; }
joomla-accordion.accordion .accordion-panel-title:focus {
box-shadow: 0px 0px 0px 2px #198df8;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
box-shadow: 0px 0px 0px 2px #198df8;
box-shadow: 0 0 0 2px #198df8;

@brianteeman
Copy link
Contributor

Oops the code changes I just commented on changing 0px to 0 should of course have been done in the scss file and not the generated css files

collapseHeader.setAttribute('area-expanded', 'false');

const collapseHeaderTitle = this.getAttribute('collapse-title') === null ? this.getAttribute('type') : this.getAttribute('collapse-title');
collapseHeader.innerHTML = collapseHeaderTitle;
Copy link
Contributor

Choose a reason for hiding this comment

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

Prone to XSS attack

createLink.className += ` ${item.getAttribute('activeClass')}`;
}
createLink.setAttribute('href', item.getAttribute('href'));
createLink.innerHTML = item.getAttribute('text');
Copy link
Contributor

Choose a reason for hiding this comment

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

Prone to XSS attack

collapseHeader.classList.add('joomla-alert--collapse-header');
collapseHeader.setAttribute('area-expanded', 'false');
var collapseHeaderTitle = this.getAttribute('collapse-title') === null ? this.getAttribute('type') : this.getAttribute('collapse-title');
collapseHeader.innerHTML = collapseHeaderTitle;
Copy link
Contributor

Choose a reason for hiding this comment

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

Prone to XSS attack

collapseHeader.setAttribute('area-expanded', 'false');

const collapseHeaderTitle = this.getAttribute('collapse-title') === null ? this.getAttribute('type') : this.getAttribute('collapse-title');
collapseHeader.innerHTML = collapseHeaderTitle;
Copy link
Contributor

Choose a reason for hiding this comment

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

Prone to XSS attack

@wilsonge wilsonge mentioned this pull request Mar 1, 2020
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.

9 participants