-
Notifications
You must be signed in to change notification settings - Fork 436
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
Fix more info in package error flash #6079
Conversation
@@ -34,3 +34,11 @@ $(function ($) { | |||
details.attr('open', null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder why this file is called packages.js
(in plural) and not package.js
? 🤔 Any clue @openSUSE/open-build-service ? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ChrisBr could answer that, but why we have package
and packages
? Maybe in the past it used to be all packages
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you think this matters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you think this matters?
consistency? I just didn't know where to put it until I found out that this was for package related things.. there is also a package-view_file.js
(in singular). It confuses me... 🙄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of this is related to anything. Its one huge javascript file that get's included on every page. However you structure it internally is up to you 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need some criteria for JavaScript file names, before this is a mess... 💡
Review app will appear here: http://obs-reviewlab.opensuse.org/ana06-more-info |
var moreInfo = $('.more_info'); | ||
moreInfo.toggleClass('d-none'); | ||
$(this).text(moreInfo.hasClass('d-none') ? 'more info' : 'less info'); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: two spaces after 'more info'. Should be one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why jshint doesn't complain? 😒
`.hidden` was been dropped in Bootrstrap 4: https://getbootstrap.com/docs/4.0/migration The Javascript for the more info was missing. Fixes #6072
checkout_code is broken... 😠 |
It looks much nicer and it is built in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tests are green meanwhile
💃 🎉 |
The Javascript for the more info was missing and
.hidden
was been dropped in Bootrstrap 4: https://getbootstrap.com/docs/4.0/migration. Also, style link in alert using Bootstrap class.Fixes #6072