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

UI: Mobile styles for faceted search #5328

Closed

Conversation

DingoEatingFuzz
Copy link
Contributor

This is part four of four for faceted search in the Nomad UI: #5235

Screenshots say it all. Just making sure nothing looks absolutely horrible at any size.

image

image

image

image

image

@DingoEatingFuzz DingoEatingFuzz mentioned this pull request Feb 14, 2019
4 tasks
@DingoEatingFuzz DingoEatingFuzz requested a review from a team February 16, 2019 00:59
@johncowen
Copy link
Contributor

johncowen commented Feb 18, 2019

Hey!

Thanks for looking at this!

I think this is generally ok. I spotted a few of things:

screenshot 2019-02-18 at 13 56 56

screenshot 2019-02-18 at 14 25 46

screenshot 2019-02-18 at 14 46 00

I've not had a big look at the code yet, on a brief look as you can imagine there are some things there that I personally don't agree with. But as long as it works well and you are happy/satisfied with what you've done here, that's the most important thing.

Hmmm, I don't usually do things like this but, it might help you to see why I often see my decisions in Consul-land as being more appropriate, and this is my reason for posting this:

https://github.com/hashicorp/consul/pull/5326/files/daa19caf719df258552cac450089b0e96d6faa81#r257325772

Could you explain why you can't use float / flex to do this? I don't understand why on the one hand you criticise the choice of a single CSS property, yet here you replace what could be done with float/flex/@media with a new javascript dependency, a bunch of conditional templating markup, duplicate variables (the breakpoint numbers are now in two places) - all in all a completely new approach to the styling of your app. It makes it very hard to take your advice on board. Even Bulma prides itself on 'No JavaScript required'

screenshot 2019-02-18 at 15 07 49

Let me know when you've fixed up the above screengrabs and I can take another look and maybe we can have a chat.

BTW where did we get with the urlencoding of "s? I'm still seeing them, I just took a re-read of the other PR's and from that I understood that it's something I shouldn't be seeing?

Thanks,

John

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants