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

XWIKI-22581: FocusCatcher input has no label #3577

Merged
merged 5 commits into from
Dec 10, 2024
Merged

XWIKI-22581: FocusCatcher input has no label #3577

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ce0f624
XWIKI-22581: FocusCatcher input has no label
Sereza7 Oct 17, 2024
0bfc2fb
XWIKI-22581: FocusCatcher input has no label
Sereza7 Oct 18, 2024
e1d9c84
XWIKI-22581: FocusCatcher input has no label
Sereza7 Oct 18, 2024
471d225
XWIKI-22581: FocusCatcher input has no label
Sereza7 Oct 18, 2024
c68e047
XWIKI-22581: FocusCatcher input has no label
Sereza7 Dec 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ...i-platform-core/xwiki-platform-oldcore/src/main/resources/ApplicationResources.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1396,6 +1396,7 @@ core.widgets.gallery.previousImage=Show previous image
core.widgets.gallery.nextImage=Show next image
core.widgets.gallery.maximize=Maximize
core.widgets.gallery.minimize=Minimize
core.widgets.gallery.index.description=Press the right and left arrow keys to quickly navigate through the images.

core.widgets.suggest.noResults=No results!
core.widgets.suggest.showResults=Go to search page\u2026
Expand Down
74 changes: 35 additions & 39 deletions ...xwiki-platform-web-war/src/main/webapp/resources/uicomponents/widgets/gallery/gallery.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
background-color: black;
max-width: 100%;
padding: 10px;
border-radius: 7px; /* Same value as @border-radius-base from Flamingo. */
/* Position relative is required because some of the inner elements have position absolute and the gallery container
must be their offset parent. */
position: relative;
Expand All @@ -10,13 +11,9 @@
/* Those width/height values can be overridden by the inline styling added with the macro parameters */
width: 620px;
height: 349px;
}

.xGallery:before {
content: "";
display: inline-block;
height: 100%;
vertical-align: middle;
display: grid;
grid-template-columns: 1fr 10fr 1fr;
grid-template-rows: 1fr 10fr 1fr;
}

.xGallery.maximized {
Expand All @@ -26,83 +23,82 @@
top: 0;
z-index: 1001;
width: 100% !important;
border-radius: 0;
}

.xGallery .currentImage {
max-height: 100%;
grid-area: 2 / 2 / 3 / 3;
align-self: center;
justify-self: center;
object-fit: scale-down;
max-width: 100%;
padding: 22px;
vertical-align: middle;
max-height: 100%;
}

/* Transparent buttons that should fill the space they've been given on the grid*/
Sereza7 marked this conversation as resolved.
Show resolved Hide resolved
.xGallery .previous, .xGallery .next,
.xGallery .maximize, .xGallery .minimize {
background-color: transparent;
border-color: transparent;
width: 100%;
height: 100%;
}

.xGallery .previous, .xGallery .next {
color: #A0A0A0;
cursor: pointer;
font-family: courier,monospace;
font-size: 32px;
font-weight: 100;
height: 124px;
line-height: 124px;
margin-top: -64px;
position: absolute;
text-align: center;
top: 50%;
width: 32px;
}

.xGallery .previous:hover, .xGallery .next:hover {
color: white;
}

.xGallery .previous {
left: 0;
grid-area: 2 / 1 / 3 / 2;
}

.xGallery .next {
right: 0;
grid-area: 2 / 3 / 3 / 4;
}

.xGallery .index {
bottom: 10px;
color: #C0C0C0;
font-family: sans-serif;
font-size: smaller;
left: 10px;
line-height: 1;
position: absolute;
grid-area: 3 / 1 / 4 / 2;
align-self: end;
}

.xGallery .loading {
background-image: url('loading.gif') !important;
}

.xGallery .focusCatcher {
background-color: black;
border: 0 none;
color: black;
height: 1px;
left: 0;
overflow: hidden;
position: absolute;
top: 0;
width: 1px;
z-index: -1;
}

.xGallery .maximize, .xGallery .minimize {
cursor: pointer;
height: 16px;
opacity: .5;
position: absolute;
right: 10px;
top: 10px;
width: 16px;
grid-area: 1 / 3 / 2 / 4;
justify-self: end;
}

.xGallery .maximize:hover, .xGallery .minimize:hover {
opacity: 1;
}

/* Elements on the left of the grid are left aligned*/
Sereza7 marked this conversation as resolved.
Show resolved Hide resolved
.xGallery .index, .xGallery .previous {
text-align: start;
}

/* Elements on the right of the grid are right aligned*/
Sereza7 marked this conversation as resolved.
Show resolved Hide resolved
.xGallery .maximize, .xGallery .minimize, .xGallery .next {
text-align: end;
}

.xGallery .maximize {
background: transparent url('maximize.gif') no-repeat scroll center;
}
Expand Down
28 changes: 16 additions & 12 deletions .../xwiki-platform-web-war/src/main/webapp/resources/uicomponents/widgets/gallery/gallery.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,25 +22,29 @@ var XWiki = (function (XWiki) {
XWiki.Gallery = Class.create({
initialize : function(container) {
this.images = this._collectImages(container);

this.container = container.update(
'<input type="text" tabindex="-1" class="focusCatcher"/>' +
'<button class="maximize" title="${escapetool.xml($services.localization.render("core.widgets.gallery.maximize"))}"></button>' +
'<button class="previous" title="${escapetool.xml($services.localization.render("core.widgets.gallery.previousImage"))}">&lt;</button>' +
'<img class="currentImage" alt="${escapetool.xml($services.localization.render("core.widgets.gallery.currentImage"))}"/>' +
'<div class="previous" title="${escapetool.xml($services.localization.render("core.widgets.gallery.previousImage"))}">&lt;</div>' +
'<div class="next" title="${escapetool.xml($services.localization.render("core.widgets.gallery.nextImage"))}">&gt;</div>' +
'<div class="index">0 / 0</div>' +
'<div class="maximize" title="${escapetool.xml($services.localization.render("core.widgets.gallery.maximize"))}"></div>'
'<button class="next" title="${escapetool.xml($services.localization.render("core.widgets.gallery.nextImage"))}">&gt;</button>' +
'<div class="index" tabindex="0" title="${escapetool.xml($services.localization.render("core.widgets.gallery.index.description"))}" aria-description="${escapetool.xml($services.localization.render("core.widgets.gallery.index.description"))}">0 / 0</div>'
);
this.container.addClassName('xGallery');

this.focusCatcher = this.container.down('.focusCatcher');
this.container.addClassName('xGallery');

/* Instead of an arbitrary element to catch focus, we use the index.
* This index already stores the current image state, might as well be responsible for providing quick controls and
* explanations about these quick controls.
* Note that wrapping the image in an interactive container to handle this would have been a good solution too.
* However, this wrapping caused the image to overflow the CSS grid vertically when in maximized mode.
* Technically I couldn't find a CSS solution to prevent this, so I decided to make do without wrapping. */
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
/* Instead of an arbitrary element to catch focus, we use the index.
* This index already stores the current image state, might as well be responsible for providing quick controls and
* explanations about these quick controls.
* Note that wrapping the image in an interactive container to handle this would have been a good solution too.
* However, this wrapping caused the image to overflow the CSS grid vertically when in maximized mode.
* Technically I couldn't find a CSS solution to prevent this, so I decided to make do without wrapping. */
// Instead of an arbitrary element to catch focus, we use the index.
// This index already stores the current image state, might as well be responsible for providing quick controls and
// explanations about these quick controls.
// Note that wrapping the image in an interactive container to handle this would have been a good solution too.
// However, this wrapping caused the image to overflow the CSS grid vertically when in maximized mode.
// Technically I couldn't find a CSS solution to prevent this, so I decided to make do without wrapping.

Multiline comments are normally used to document functions (methods) and classes (or types in general).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't have a code style for javascript AFAIR. I assumed it'd be best to use the same comment style as the one we use for CSS. It would be interesting to keep trace of this rule so that new contributors won't make the same mistake. I created the draft doc https://dev.xwiki.org/xwiki/bin/view/Drafts/Javascript%20Code%20Style/ for now.

Reference of where we can already see this codestyle in action

Copy link
Member

Choose a reason for hiding this comment

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

I assumed it'd be best to use the same comment style as the one we use for CSS.

JavaScript is a programming language, unlike CSS, so I don't think it makes sense to look at CSS code styles for defaults. The default should be the Java code style IMO.

this.focusCatcher = this.container.down('.index');
this.focusCatcher.observe('keydown', this._onKeyDown.bindAsEventListener(this));
this.container.observe('click', function() {
this.focusCatcher.focus();
}.bind(this));

this.container.down('.previous').observe('click', this._onPreviousImage.bind(this));
this.container.down('.next').observe('click', this._onNextImage.bind(this));
this.container.observe('click', function() {
this.focusCatcher.focus();
}.bind(this));

this.currentImage = this.container.down('.currentImage');
this.currentImage.observe('load', this._onLoadImage.bind(this));
Expand Down