Remove the unwnated message of 'Please agree to the term of use' #540
Conversation
|
|
|
Hi, @YashAgrawal9265. Thank you for contributing to the project! This message should appear every time you log into the site without having agreed to the terms of use. Did you agree with the terms of use? If not, could you agree to them and then check if the message appears. Let me know if you still see that message after agreeing to the terms of use. |
|
Hi, @suecarmol . I had also checked that message after agreeing to the term of use. The message did not appears |
|
@suecarmol Yash and I chatted about this a little on the Phabricator ticket and I suggested moving it entirely because the yellow message is always present. It felt redundant to also be showing the one-off blue notice when the yellow one is right below it anyway: |
YashAgrawal9265
force-pushed
the
master
branch
from
ac842ed
to
5893449
Compare
|
I had made this commit 9dc78c6. This has pass all the checks .Please review it and suggest if any change needed... |
|
Thanks for the information. Can I get a link to the Phabricator ticket? I can't seem to find it in our Kanban board |
|
I had not metioned this task on the phabricator , so phabricator ticket is not available |
|
I misspoke, we spoke via email, not Phabricator :) |
TWLight/static/css/local.css
Outdated
| .homeBtn:hover { | ||
| background-color:#337ab7; | ||
| border-color:#337ab7; | ||
| color: #fff; | ||
| } |
There was a problem hiding this comment.
I'm not sure about this change here. I believe we should keep the button's default CSS. @Samwalton9 @jsnshrmn what do you think?
This is the current behavior of the default buttons on hover:
This is the changed behavior of the default buttons on hover:
If we keep the changes you made, I think the CSS class should be renamed to something different, like default-btn-override:hover
There was a problem hiding this comment.
It's not clear to me what problem the css change is solving or how it relates to the pr. I'd exclude the change.
There was a problem hiding this comment.
Should I rename the CSS class right now???
There was a problem hiding this comment.
Actually It's my mistake that I had put this CSS change in this PR. Instead I should file a another PR for this CSS change.
If you don't like this change, I can remove this...
There was a problem hiding this comment.
Let's back up a step: why did you change the css?
There was a problem hiding this comment.
Is there a phab task for the problem that the css change is trying to solve?
There was a problem hiding this comment.
I thought that this change would look nice and more attractive than the default CSS
There was a problem hiding this comment.
No, but I had discuss this change with Sam on email
There was a problem hiding this comment.
Ah, yeah, I can't really evaluate in that case, but it doesn't seem like an obvious improvement to me since it's a common bootstrap UI metaphor to simply darken the existing color when hovering over an interactive UI element instead of changing colors completely. Will wait to hear back from @Samwalton9 either for evaluation or for more information. I agree with @suecarmol that if we want to accept the change, the class name shouldn't be "homeBtn".
There was a problem hiding this comment.
Agreed that the default behaviour is fine, I don't see a need to add complexity for such a minor change.
|
I had removed the changes of CSS which I had made earlier. |
Description
Describe your changes in detail following the commit message guidelines
Rationale
Phabricator Ticket
How Has This Been Tested?
Screenshots of your changes (if appropriate):
Types of changes
What types of changes does your code introduce? Add an
xin all the boxes that apply: