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

medialist.php - multiple validation errors #30

Closed
wants to merge 1 commit into from
Closed

medialist.php - multiple validation errors #30

wants to merge 1 commit into from

Conversation

JustCarmen
Copy link
Contributor

Hi Greg,

I have made the medialist page html valid. Therefore I had to change multiple files and stylesheets. I hope you will accept the changes.

@@ -79,7 +79,7 @@
<?php echo WT_I18N::translate('Folder'); ?>
</td>
<td class="optionbox wrap width25">
<?php echo select_edit_control('folder', $folders, null, $folder); ?>
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think this change is correct. The list of folders is permitted to have an empty entry. For example, if you have 3 media files:

alpha.jpg
foo/beta.jpg
foo/gamma.jpg

Then the list of folders will be array('', 'foo'). Selecting the first entry will give alpha.jpg and selecting the second will give beta.jpg and gamma.jpg

Copy link
Owner

Choose a reason for hiding this comment

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

If the list of folders is empty (i.e. all media objects are in the top-level folder), then perhaps we need to hide the list of folder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to W3C the option value could not be empty:

W3C: "Element option without attribute label must not be empty."

That's why I choose to set the Media directory name as first option. A second reason for this is that I think it is confusing for visitors that the first option is blank while it can have items.

I agree with you that if the list of folders is empty then we could hide the folder list. But this is only possible if the media items are loaded immediately when clicking the menu link. Currently this is not the case.

@fisharebest
Copy link
Owner

Hi Carmen - sorry it has taken me so long to find the time to look at this.

It seems mostly right, but I have a few comments...

@JustCarmen
Copy link
Contributor Author

Greg, no problem. I know you are very busy and this was not something with high priority. But it was something that bothers me for a while. In my own theme I use some jquery to get rid of the multiple id's for example. But it really should be changed in the core, so thats why I decided to update the code myself.

I wait for your answers and then I can update the code to make the improvements if you wish.

@fisharebest
Copy link
Owner

I've merged this change. I've also made a few other changes to medialist. If I have missed or broken anything, let me know!

@HRN65 HRN65 mentioned this pull request Apr 13, 2020
@ghost ghost mentioned this pull request Aug 9, 2020
@webtrees-pesz webtrees-pesz mentioned this pull request Aug 18, 2023
@makitso makitso mentioned this pull request Oct 21, 2023
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.

None yet

2 participants