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

Improve web UI #20

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

Improve web UI #20

wants to merge 4 commits into from

Conversation

slatian
Copy link
Contributor

@slatian slatian commented Dec 3, 2022

Includes the following patches:

  • .View variable in templates (to make some deduplication possible)
  • Deduplicated the navigation and search-form
  • The tagline is now a paragraph instead of an h2

NOT included are (these will be followup PRs):

…o) and some minor accessibility improvements (hopefully)

Note for potential upstreaming: This depends on commit eb4aed02e98083892afbf805ec628a2a20951b58 for passing the .View variable to the templates
…ph with a fancy look instead of a pretty pointless headline
@cblgh
Copy link
Owner

cblgh commented Dec 6, 2022

@slatian When adding interface changes, can you also include a screenshot in the PR? Simplifies reviewing a ton!

@slatian
Copy link
Contributor Author

slatian commented Dec 6, 2022

@cblgh This will not affect the visuals of Lieu (I should have mentioned that).

The changes here make it easier to modify (as in customizing/theming) and extend Lieu Templates (needed by my 404 page patch and the error messages patch), those will introduce some minor changes. (The "big" one of them being an additional margin on paragraphs to make the error messages readable)

@cblgh
Copy link
Owner

cblgh commented Dec 7, 2022

@slatian ah! i admit i punted on reviewing the code (having already gone through the other PRs i was feeling done for the moment hehe). thanks for the extra context! i'll review this next Lieu pass i do

@cblgh
Copy link
Owner

cblgh commented Feb 22, 2023

@slatian hiyo! prompted by #21 i'll be returning to reviewing this on Lieu. any idea where this PR was at? you mention some things not being included, is that still the case? or can this be merged as-is after the merge conflicts have been resolved

@slatian
Copy link
Contributor Author

slatian commented Feb 22, 2023

This one is a little template cleanup that sets the stage for the patches that I mentioned above were not included. The 404 page and error messages depended on the advanced search feature which wasn't merged at the time, that is why I explicitly mentioned that they weren't included, I'll submit both once I have a little more time on hand.

Lot of words nothing said: This one can be merged.

Edit: I was waiting with the other patches so that I can submit the others fir a fresh rebase (Also they both depend on the view variable as far as I remember). I'm doing it this way to avoid another mega PR with multiple features in one "blob"

@slatian
Copy link
Contributor Author

slatian commented Aug 20, 2023

I'm still interested in pushing the patches that depend on this once it gets merged. In case you want them as bigger PR package that is fine on my end, too. (the reason for splitting was that more than one feature depends on the functionality in here)

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.

2 participants