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

fix(refs DPLAN-12407): fix spacings and alignments #3699

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

hwiem
Copy link
Contributor

@hwiem hwiem commented Sep 19, 2024

Ticket

https://demoseurope.youtrack.cloud/issue/DPLAN-12407/Grundkarten-Auge-Icon-und-Bearbeiten-sind-verrutscht-und-da-gibt-keine-Padding-fur-Ubersichtskarte

Lots of spacings and alignments are fixed in the planning documents view:

  • table header items are aligned properly
  • icons in the table are aligned properly
  • spacing above and below items is consistent
  • the "Übersichtskarte" heading and the hint are displayed above the select
  • the "Kartenebenen" legend is changed to an h2 to better match (and visualize) the logical hierarchy

How to review/test

PR Checklist

  • Link all relevant tickets
  • Move the tickets on the board accordingly

c-at-item__row was applying a width of 100%-31px,
which lead to the table header elements being displaced
to the left
- the logical hierarchy is still a bit messed up, but the "gislayer"
heading is more important than its subheadings (overlays, base layers),
and the h2 was missing in this view altogether
- lots of spacings (below/above buttons and other elements) and alignments
(of icons) were not consistent or shifted
- the checkbox in the table is given a margin-right of 27px so it aligns with the
icon in the table header, but this does not work for mobile devices - the entire
view is not optimized for mobile devices, and this should be fixed separately
@hwiem hwiem added the review:frontend PRs that are missing review(s) from a FE perspective label Sep 19, 2024
@hwiem hwiem self-assigned this Sep 19, 2024
client/js/components/map/admin/AdminLayerList.vue Outdated Show resolved Hide resolved
{{ Translator.trans('map.base.minimap') }}
</h3>
<div class="w-2/3">
<div>
<p class="font-size-small">
Copy link
Contributor

Choose a reason for hiding this comment

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

We can already use tw classes on font size, they are configured to match our font scale.

Suggested change
<p class="font-size-small">
<p class="text-sm">

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will change the font-size from 15px to 14px. Do we want that?

Copy link
Contributor

Choose a reason for hiding this comment

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

sure that it is 15px? the selector is defined here, and the variable is imported from demosplan-ui where it gets generated from this token which aliases to fontSize.scale.2 which is also 14px. Am i missing something, or does the value get overridden somewhere?

client/js/components/map/admin/AdminLayerList.vue Outdated Show resolved Hide resolved
client/js/components/map/admin/AdminLayerListItem.vue Outdated Show resolved Hide resolved
@@ -248,7 +248,7 @@ export default {
return {
drag: false,
preventActiveFromToggeling: false,
iconClass: 'fa with--40 u-ml u-mr',
iconClass: 'mb-0 u-ml',
Copy link
Contributor

Choose a reason for hiding this comment

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

TW margin would be:

Suggested change
iconClass: 'mb-0 u-ml',
iconClass: 'mb-0 ml-4',

A note on that: as i see, TW does not feature default steps in their margin- or padding scale, that means we cannot go just mt or p. Do we want that? Then i could add those classes to the TW config.

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 think it would be nice to have mb instead of mb-1 (etc.) 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok! However as i see the u-m(b|t|r)-0_5 (12px) is the most used spacing step by far, so i'd opt to make that the default step.

after removing the legend, we should remove the fieldset,
too, since a fieldset without legend is less accessible
the only styling really needed here is display: inline-block
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review:frontend PRs that are missing review(s) from a FE perspective
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants