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-10871: Misalignments in the class editor on mobile #3885

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

XWIKI-10871: Misalignments in the class editor on mobile #3885

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8707512
XWIKI-10871: Misalignments in the class editor on mobile
tkrieck Feb 10, 2025
6a4a4a6
XWIKI-10871: Misalignments in the class editor on mobile
tkrieck Feb 18, 2025
a776254
Merge branch 'xwiki:master' into XWIKI-10871
tkrieck Feb 18, 2025
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
24 changes: 14 additions & 10 deletions ...ingo-skin/xwiki-platform-flamingo-skin-resources/src/main/resources/flamingo/editclass.vm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,16 +125,20 @@ $xwiki.jsfx.use('js/xwiki/editors/dataeditors.js', true)##
* Inline form for adding a new property to the class.
*#
#macro(addPropertyForm)
<div id="add_xproperty">
$services.icon.renderHTML('add')
<label for="propname" class="property-name-label">$services.localization.render('core.editors.class.addProperty.name.label')</label>
<input type="text" id="propname" name="propname" value="name" size="20" class='withTip'/>
<label for="proptype" class="property-type-label">$services.localization.render('core.editors.class.addProperty.type.label'):</label>
<select id="proptype" name="proptype" size="1">
#foreach($prop in $xwiki.metaclass.properties)
<option value="${prop.name}">${escapetool.xml($prop.prettyName)}</option>
#end
</select>
<div id="add_xproperty" class="add_xproperty">
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be sufficient to target this element with the existing ID attribute? Other CSS rules in the same file targeting the same element use this ID, too. Or is there any reason to prefer using classes? If we introduce a new class, I would prefer to follow our naming conventions with add-xproperty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd leave IDs for JS reference only, or as much as possible. My reason is that, while it's possible to target it with CSS, the specificities of IDs are higher than classes. See: https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_cascade/Specificity

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a commonly used pattern to not use IDs in CSS? I'm asking because in XWiki, I think we're currently not following this pattern. So maybe there should be a proposal to add this to our code style? To me, it seems quite inconsistent to have a CSS file with selectors for both the ID and the class that are both only on a single element. I'm not asking to completely change them, but to me the only good reason to have this inconsistency and not use the ID as in the rest of the CSS would be that we say that IDs shouldn't be used in CSS, and therefore we migrate our code to the new style whenever we touch it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this a commonly used pattern to not use IDs in CSS?

It's very common, the lower the specificity of each CSS rule means that overwriting it will be easier for different cases like hovering, disabling for example. Also, there's better portability (defining one rule and using it across many different places).

To me, it seems quite inconsistent to have a CSS file with selectors for both the ID and the class that are both only on a single element.

You have a good point here. If you think it's more coherent, I'll port this implementation to use the ID, but open a proposal to change the code style. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with either using the ID or introducing a new class (but preferably using kebab-case, so add-xproperty), but for the second option there would need to be a proposal to add this to the code style. You could open the proposal, and then we wait a few days, and if nobody objects, the PR can be merged.

<div>
$services.icon.renderHTML('add')
<label for="propname" class="property-name-label">$services.localization.render('core.editors.class.addProperty.name.label')</label>
<input type="text" id="propname" name="propname" value="name" size="20" class='withTip'/>
</div>
<div>
<label for="proptype" class="property-type-label">$services.localization.render('core.editors.class.addProperty.type.label'):</label>
<select id="proptype" name="proptype" size="1">
#foreach($prop in $xwiki.metaclass.properties)
<option value="${prop.name}">${escapetool.xml($prop.prettyName)}</option>
#end
</select>
</div>
<span class="buttonwrapper">
<input type="submit" class="button" value="$services.localization.render('core.editors.class.addProperty.submit')" name="action_propadd"/>
</span>
Expand Down
6 changes: 6 additions & 0 deletions ...orm-web/xwiki-platform-web-war/src/main/webapp/resources/js/xwiki/editors/dataeditors.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,12 @@ The default state is collapsed for xobjects and xproperties, but expanded for xc
background-image: url("$xwiki.getSkinFile('icons/datamodel/password.png')");
}

.add_xproperty {
display: flex;
gap: 1em;
flex-wrap: wrap;
}

#body #xwikiclassproperties #add_xproperty label,
#body #xwikiobjects #add_xobject label {
margin: 0;
Expand Down