tuwien-csd / damap-frontend

MIT License
11 stars 10 forks source link

Global: Update tables, search fields and form-fields #336

Closed ValentinFutterer closed 2 weeks ago

ValentinFutterer commented 1 month ago

Description

Refactoring

What does this PR do?

I created a global table css-class, that gives all tables in the project the zebra like appaerance. The dmp-table shown below got some additional styling. The light blue is the new color that appears when hovering over a table cell, this color changes with the color palette. I restyled the search fields and created a reusable component for them. Most notably they now use the outline appearance and are slimmer.

image

They same is true for all the other form fields, i also made outline the default appearance. The only exception is the textarea wrapper, which i left filled and only with slightly reduced density. All outlined form-fields now have a primary colored border by default.

Before

image

After

image

There are also numerous small code improvements throughout the whole PR.

Breaking changes

Code review focus

Dependencies

Checks

closes GH-319

ValentinFutterer commented 1 month ago

Is the menu supposed to have a blue background?

Yes, since the original gray background didnt look got on the zebra table, there was no contrast. I decided to use the secondary-container color, sincethe m3 styleguide describes its usage like this Less prominent fill color against surface, for recessive components like tonal buttons. We could use another color though, if you have a suggestion.

Person search result items take up a quite large space now. This was not the case before. before:

Good catch. I couldnt find the reason on a first look through, i will fix it.

ValentinFutterer commented 1 month ago

app-input-wrapper only allows for a single line. For longer user input, app-textarea-wrapper is better suited. So wherever a user might want to add some longer text and not only a few words, app-textarea-wrapper should be used. This may not be part of this issue, feel free to shelve and create a new issue.

Laura wrote a similar comment, i changed app-input-wrapper to app-textarea-wrapper where it makes sense to do so.

For consistency reasons, do we want to align this throughout the application to use the same style everywhere? I.e. outlined instead of filled for app-input-wrapper as well as app-textarea-wrapper?

That is a good question. I left the app-textarea-wrapper as filled, to keep it distinct, but we could just as well change it to outlined.

ValentinFutterer commented 3 weeks ago

First review: Adjusting the border thickness looks good but, I noticed that when focusing on the input field, the text inside becomes harder to read.

The default outline style would behave the same, since the component doesnt reserve extra space on top for the label, like the subscript in a form field does.

I see two options for the label text problem

lpandath commented 3 weeks ago

First review: Adjusting the border thickness looks good but, I noticed that when focusing on the input field, the text inside becomes harder to read.

The default outline style would behave the same, since the component doesnt reserve extra space on top for the label, like the subscript in a form field does.

I see two options for the label text problem

* add margins on top of every form field by default as global css, like is done by angular material via subscript

* change every componenet individually (could also be done in the split screen rework)

After testing it, I think it would likely be more efficient to add this margin (top: 10px) in the global CSS, specifically targeting the first form field in each section. This way, it only affects the layout where needed without applying margin to all fields. It should also keep the component stable through future upgrades.

testing; formfield

sonarcloud[bot] commented 2 weeks ago

Quality Gate Failed Quality Gate failed

Failed conditions
29.6% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud