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

Achievements: add icons and isClickable check (fixes #3251) #3256

Merged
merged 11 commits into from
Feb 26, 2019

Conversation

svlesiv
Copy link
Member

@svlesiv svlesiv commented Feb 22, 2019

screen shot 2019-02-22 at 1 26 12 pm

Copy link
Contributor

@singharpita singharpita left a comment

Choose a reason for hiding this comment

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

@YuserahN looks good. but the cursor pointer is only shown when there is both resource and description attached. in other case when there is only resource or description cursor pointer is not showing

screenshot from 2019-02-24 21-37-55

@yubarajpoudel
Copy link
Contributor

@singharpita agreed. @svlesiv I think it is better to show cursor when hovered through the icon either it is both or one between description and resources.

@svlesiv
Copy link
Member Author

svlesiv commented Feb 24, 2019

@singharpita and @Yuviii, thank you for noticing this bug. I've fixed it. Now it should work properly.

Copy link
Member

@lmmrssa lmmrssa left a comment

Choose a reason for hiding this comment

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

@svlesiv We want to have generic icon to show that it is collapsable. You do not need to add two separate icons. Also If there is no resource or description then we do not want it to be collapsable at all.

@svlesiv
Copy link
Member Author

svlesiv commented Feb 25, 2019

Hi @lmmrssa, thanks for the review. I will try to fix it.

@svlesiv
Copy link
Member Author

svlesiv commented Feb 25, 2019

screen shot 2019-02-25 at 11 49 30 am

@svlesiv
Copy link
Member Author

svlesiv commented Feb 25, 2019

Or it can be other icons:

screen shot 2019-02-25 at 12 12 48 pm

@paulbert
Copy link
Member

@svlesiv I think having the icon to the right of the title would be preferable so all of the titles line up & the title is the first thing you read. Leaning towards the chevron arrow, also.

@svlesiv
Copy link
Member Author

svlesiv commented Feb 26, 2019

Sure, I will fix it.

@svlesiv svlesiv closed this Feb 26, 2019
@svlesiv svlesiv reopened this Feb 26, 2019
@paulbert paulbert merged commit 5d6130d into master Feb 26, 2019
@lmmrssa lmmrssa deleted the 3251-attachments-icon branch February 28, 2019 10:58
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.

6 participants