Skip to content

add search bar to header#71

Merged
wipascal merged 4 commits intomainfrom
feature/searchbar
Aug 18, 2021
Merged

add search bar to header#71
wipascal merged 4 commits intomainfrom
feature/searchbar

Conversation

@wipascal
Copy link
Contributor

No description provided.

@wipascal wipascal requested a review from saemideluxe August 17, 2021 12:45
hg.A(
hg.SPAN(
company,
style=f"position: absolute; left: {hg.If(searchbar, hg.BaseElement('50 %'), hg.BaseElement('5rem'))}",
Copy link
Member

@saemideluxe saemideluxe Aug 18, 2021

Choose a reason for hiding this comment

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

This will not work as expected, because formatted strings in python are not able to render hg.BaseElement elements correctly (mainly because the string gets interpolated immediately and the __format__/__str__ function is used to convert objects to strings instead of hg.render). I am aware that a notation as used here would be much nicer. There is already an open issue to add an API for this in htmlgenerator: basxsoftwareassociation/htmlgenerator#6

So now I am even more motivated to add the feature ;)

Copy link
Member

Choose a reason for hiding this comment

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

If you have some input on how the implementation or the usage for this feature should look like, feel free to comment on the issue in htmlgenerator.

Copy link
Member

Choose a reason for hiding this comment

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

screenshot-1629258443_626x50

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I added a first version for this to htmlgenerator (released already). With the current implementation the implementation would look like this:

style=hg.format("position: absolute; left: {}", hg.If(searchbar, "50%", "5rem"),

It is not perfect yet, compared to f-strings. However, it is fully compatible to the python string formatting system. The f-string notation does not really allow for lazy evalutation of its content, it is immediatley evaluated as part of the string literal.
There could be a solution with a custom function like hg.f("position: absolute; left: {hg.If(searchbar, '50%', '5rem')}") (note the missing f-string specifier) and dynamically evaluates that string when we render. But we would have the problem on how we can capture the scope where the string is defined and pass it to the place where the string is evaluated.

Copy link
Member

@saemideluxe saemideluxe Aug 18, 2021

Choose a reason for hiding this comment

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

Btw. hg.If works with strings directly. hg.BaseElement is only required if you want to have a list of things (without a wrapping hg.DIV or similar).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright thanks! it should work now

platform,
_class="bx--header__name",
style="font-weight: 400", # override carbon design
href=hg.F(lambda c: c["request"].META["SCRIPT_NAME"] or "/"),
Copy link
Member

Choose a reason for hiding this comment

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

In case you wondered about this line: The WSGI protocol allows us to "mount" an application on something else than the root of a domain. Se we can e.g. have https://website.com/app1 and https://website.com/app2 and both running with on the same code and seeing themselfes as beeing on the root. All django URL resolving will work correctly as well with this. But when we use our own constructed URLs (like here, because we just want the root-url of this django instance and we don't know the name of that URL) then we have to take care of it.

@wipascal wipascal requested a review from saemideluxe August 18, 2021 06:36
Copy link
Member

@saemideluxe saemideluxe left a comment

Choose a reason for hiding this comment

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

Thanks, new search bar looks great.

@wipascal wipascal merged commit 7febff7 into main Aug 18, 2021
@wipascal wipascal deleted the feature/searchbar branch August 18, 2021 08:21
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