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

Traces#index: Introduce tab navigation, fix tag-filter #3034

Closed
wants to merge 1 commit into from

Conversation

tordans
Copy link
Contributor

@tordans tordans commented Jan 2, 2021

  • introduce bootstrap tabs to switch the views
  • introduce content_for :heading_class to remove the padding below the bootstrap tabs
  • update the rss-image to use a svg, adopted from https://icons.getbootstrap.com/icons/rss/ (without the outer border)
  • move rss- and new-button away from the view-switching actions
  • the @tag logic was broken. introduce new link to remove the tag-filter; the tabs keep the filter once given
  • use &. syntax nil-safety so we can remove @display_name

I will wait for a first review of this.

Still to do:

  • Double Check the usage of params[:tag] in the views. Did I introduce a security problem here? If so, what is the smartest way to solve it? – Will need to look into this; but maybe someone already has the answer …
  • Check specs
  • Add translations for the two new strings (hint about how to this is welcome – just add it to en.yml and the other translations will come automatically?)

The remove-tag-feature is not ideal IMO, but on the other hand it was broken ATM and nobody noticed so it was probably not that important and IMO not worth adding even more complexity.

Screenshots

gps – logged-in – someone-else
gps--logged-in--someone-else

gps – logged-in – all
gps--logged-in--all

gps – logged-in – my
gps--logged-in--my

gps – logged-out – someone-else
gps--logged-out--someone-else

gps – logged-out – all
gps--logged-out--all

gps – logged-in – someone-else – tag filter active
gps--logged-in--someone-else--tag

gps – logged-in – all – tag filter active
gps--logged-in--all--tag

gps – logged-in – my – tag filter active
gps--logged-in--my--tag

gps – logged-out – someone-else – tag filter active
gps--logged-out--someone-else--tag

gps – logged-out – all – tag filter active
gps--logged-out--all--tag

- introduce bootstrap tabs to switch the views
- introduce `content_for :heading_class` to remove the padding below the bootstrap tabs
- update the rss-image to use a svg, adopted from https://icons.getbootstrap.com/icons/rss/ (without the outer border)
- move rss- and new-button away from the view-switching actions
- the `@tag` logic was broken. introduce new link to remove the tag-filter; the tabs keep the filter once given; use params[:tag] directly in the view
- use `&.` syntax nil-safety so we can remove `@display_name`
@gravitystorm
Copy link
Collaborator

  • (hint about how to this is welcome – just add it to en.yml and the other translations will come automatically?)

Yes 😄

@gravitystorm
Copy link
Collaborator

* Double Check the usage of `params[:tag]` in the views.

From a quick review, I don't see anything obviously wrong. Rails is very good about avoiding XSS and similar problems, so long as you avoid using any of the utilities that mess around with string safety (like raw, .html_safe, etc). I can double-check when the PR is ready for review.

@gravitystorm gravitystorm marked this pull request as draft January 6, 2021 18:04
@gravitystorm
Copy link
Collaborator

I've marked this as draft due to the test failures.

@gravitystorm
Copy link
Collaborator

I've included the patch from @migurski that got the tests working, along with some changes from myself (instead of adding them here as review notes) in #3226 .

Therefore I'm closing this as superceded by #3226

Thanks for the initial work @tordans , I think the tabs look great and I can see us using them in other situations too!

@migurski
Copy link
Contributor

Nice to see this make it in!

@tordans
Copy link
Contributor Author

tordans commented Jun 17, 2021

Yes, thanks for finishing this!

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.

None yet

3 participants