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.

Sign up for GitHub

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

Jump to bottom

Fixing accessibility problems and HTML validation errors in built-in themes (Lombiq Technologies: OCORE-83) #11243

Merged
merged 7 commits into from
Jan 22, 2024
Merged

Fixing accessibility problems and HTML validation errors in built-in themes (Lombiq Technologies: OCORE-83) #11243

merged 7 commits into from
Jan 22, 2024

Conversation

DemeSzabolcs
Copy link
Contributor

@DemeSzabolcs DemeSzabolcs commented Feb 22, 2022 •
edited
Loading

This PR doesn't include fixes for: 25:55 error The autoplay attribute is not allowed on <video> no-autoplay HTML validation error in The Coming Soon Theme, because I think the auto-playing video is a crucial part of that theme. Also, there are overlays on the video, and it's in the background, so it's not that disturbing.

@dnfadmin
Copy link

dnfadmin commented Feb 22, 2022 •
edited
Loading

CLA assistant check
All CLA requirements met.

@Skrypt
Copy link
Contributor

Skrypt commented Feb 22, 2022

Please open a PR on the appropriate repository else we will restart fixing those when we will update these themes later on.

@hishamco
Copy link
Member

I don't think this PR should go in OC

@Piedone
Copy link
Member

Piedone commented Feb 22, 2022

Following up from the issue, I think:

In any case, all of these issues need to be fixed one way or another.

@DemeSzabolcs
Copy link
Contributor Author

DemeSzabolcs commented Feb 24, 2022 •
edited
Loading

I created PRs on the appropriate repositories:
The Coming Soon theme: StartBootstrap/startbootstrap-coming-soon#62
The Agency theme: StartBootstrap/startbootstrap-agency#323
The Agency theme insufficient color contrast issue: StartBootstrap/startbootstrap-agency#322
The Default theme insufficient color contrast issue: StartBootstrap/startbootstrap-bare#58

@Piedone
Copy link
Member

Piedone commented Jan 11, 2024

I don't think there's any point in waiting more for those theme projects that didn't react to your issues. Please wrap this up with everything that's readily usable, including updating Agency, since that was fixed (StartBootstrap/startbootstrap-agency#323).

@Piedone Piedone marked this pull request as draft January 11, 2024 18:50
@Piedone Piedone removed the notready label Jan 11, 2024
@DemeSzabolcs
Merge remote-tracking branch 'lombiq/main' into issue/OCORE-83
6856cda 
# Conflicts:
#	src/OrchardCore.Themes/TheAgencyTheme/Assets/dist/index.html
@DemeSzabolcs
Copy link
Contributor Author

including updating Agency

I just checked, I already updated it here in the way that the PR was accepted in its own repository.

@DemeSzabolcs DemeSzabolcs marked this pull request as ready for review January 20, 2024 23:21
Piedone
Piedone requested changes Jan 21, 2024
View reviewed changes
Copy link
Member

@Piedone Piedone left a comment

Choose a reason for hiding this comment

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

This does not fix #11241, right?

Since due to the autoplay video remaining this does not fully fix #11242 please factor that problem out to its own issue.

src/OrchardCore.Themes/TheAgencyTheme/Recipes/Snippets/footer.html Outdated Show resolved Hide resolved
src/OrchardCore.Themes/TheBlogTheme/Views/Content-Category.Summary.liquid Show resolved Hide resolved
@DemeSzabolcs
Copy link
Contributor Author

DemeSzabolcs commented Jan 21, 2024 •
edited
Loading

@Piedone

This does not fix #11241, right?

No because of these:
StartBootstrap/startbootstrap-coming-soon#62
StartBootstrap/startbootstrap-agency#322
StartBootstrap/startbootstrap-bare#58

Since due to the autoplay video remaining this does not fully fix #11242 please factor that problem out to its own issue.

We talked about this internally way back. The video is in the background, so there is no way to start it manually. And the conclusion was that it could remain.

@Piedone
Copy link
Member

Piedone commented Jan 21, 2024

OK!

@Piedone Piedone merged commit 9cc438e into OrchardCMS:main Jan 22, 2024
3 checks passed
Skrypt pushed a commit that referenced this pull request Jan 25, 2024
@DemeSzabolcs @Skrypt
Fixing accessibility problems and HTML validation errors in built-in …
f5d9e68 
…themes (#11243)
Skrypt added a commit that referenced this pull request Jan 25, 2024
@Skrypt
Fixing accessibility problems and HTML validation errors in built-in …
3da7ae4 
…themes (#11243)

Cleanup ISmsService (#15142)

Fix TheAdminTheme layout margin and padding (#15143)

Fix SectionDisplayDriver prefix (#15123)

Prefill template name when creating a template. (#15145)

Set the User Localization feature priority

Fix issue with default culture not selected

When currentUserCulture is null or supportedCulture doesn't contain currentUserCulture.

Update the height of the admin content (#15153)

Eliminate the anti-discovery pattern in Elasticsearch (#15134)

Renaming and cleaning up search services (#15156)

mkdocs-material 9.5.5

User Timezone settings refresh

Originally from Hisham

revert manifest
hishamco pushed a commit that referenced this pull request Feb 1, 2024
@DemeSzabolcs @hishamco
Fixing accessibility problems and HTML validation errors in built-in …
ecc7ff1 
…themes (#11243)
urbanit pushed a commit to urbanit/OrchardCore that referenced this pull request Mar 18, 2024
@DemeSzabolcs @urbanit
Fixing accessibility problems and HTML validation errors in built-in …
435e94e 
…themes (OrchardCMS#11243)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Piedone Piedone Piedone approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@DemeSzabolcs @dnfadmin @Skrypt @hishamco @Piedone