Skip to content

Murad/12189 feedback close btn - #247

Merged
Agrendalath merged 5 commits into
open-craft:masterfrom
murad-hubib:murad/12189-feedback-close-btn
Jan 13, 2020
Merged

Murad/12189 feedback close btn#247
Agrendalath merged 5 commits into
open-craft:masterfrom
murad-hubib:murad/12189-feedback-close-btn

Conversation

@murad-hubib

@murad-hubib murad-hubib commented Nov 7, 2019

Copy link
Copy Markdown
Contributor
  • Added close button icon in HTML i-e "×" and removed css definition

How to test:

  • Select an option from options and a feedback window will be open
  • If not, you can click on red/green icon to see it
  • The window should display a close button
  • There is a change in location of code so apparently it will not effect the visuals

Ticket:
https://edx-wiki.atlassian.net/browse/MCKIN-12189

@xitij2000 Please review, squash and merge

@Agrendalath

Copy link
Copy Markdown
Member

Thank you for the contribution, @murad-hubib. I'll review this today.

@Agrendalath
Agrendalath self-requested a review November 25, 2019 14:34
@murad-hubib

Copy link
Copy Markdown
Contributor Author

Thank you for the contribution, @murad-hubib. I'll review this today.

Thanks for heads up @Agrendalath

@Agrendalath

Copy link
Copy Markdown
Member

@murad-hubib, I tested this on Ironwood devstack and it didn't show up until I manually disabled display: none rule for the div in browser. Should I configure something to see it without such workaround?

This is how it looks on the master version.
master

This is how it looks after the changes.
changed
Is it supposed to look like this? It's pretty odd to have two "close" buttons there.

@murad-hubib

Copy link
Copy Markdown
Contributor Author

@Agrendalath Thanks for pointing out the redundancy of icons. I have removed the icon from html and now i will add it from mcka-theme. Please go ahead.

P.S: Yes the close icon is by default is hidden in xblock.

@Agrendalath

Copy link
Copy Markdown
Member

@murad-hubib, unfortunately I won't have time to review this today. I can do it on Sunday. Please resolve the conflicts in the meantime.

Comment thread problem_builder/public/css/questionnaire.css
@Agrendalath

Copy link
Copy Markdown
Member

@murad-hubib, please resolve the conflict - it has been changed from div to button in #248.

@Agrendalath

Copy link
Copy Markdown
Member

Also please bump the version in setup.py.

@murad-hubib

Copy link
Copy Markdown
Contributor Author

@Agrendalath Resolved conflict and bumped the version

@Agrendalath

Copy link
Copy Markdown
Member

@murad-hubib, thanks! I'll check this tomorrow.

@Agrendalath Agrendalath left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@murad-hubib, okay, so I tested this on my devstack and it looks like the following:

Default theme (in Studio):
before

Custom theme (with your change CSS for close button checked out):
after

The icon seems to be missing for me there, but I have an old devstack, so this might be on issue on my side. Could you please confirm that it's working correctly for you?

Comment thread problem_builder/public/css/questionnaire.css
@murad-hubib
murad-hubib force-pushed the murad/12189-feedback-close-btn branch from e01348e to 2d35476 Compare January 6, 2020 14:41

@Agrendalath Agrendalath left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@murad-hubib, I recompiled the theme with your other PR, but the duplicated icons still appear without it (e.g. when manually enabled in studio). I left two suggestions to make it more straightforward.

Comment thread problem_builder/public/css/questionnaire.css
Comment thread problem_builder/templates/html/tip_choice_group.html

@Agrendalath Agrendalath left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

  • I tested this: tested it on local devstacks with custom theme
  • I read through the code
  • I checked for accessibility issues
  • Includes documentation

@Agrendalath

Copy link
Copy Markdown
Member

@murad-hubib, thanks for explaining the approach. Please bump the version in setup.py. I cannot do this myself, because GitHub is marking setup.py as up-to-date for some reason.

@murad-hubib

Copy link
Copy Markdown
Contributor Author

@murad-hubib, thanks for explaining the approach. Please bump the version in setup.py. I cannot do this myself, because GitHub is marking setup.py as up-to-date for some reason.

@Agrendalath Thanks for the understanding. I have bumped the version please go ahead.

@Agrendalath
Agrendalath merged commit cae6611 into open-craft:master Jan 13, 2020
@Agrendalath

Agrendalath commented Jan 13, 2020

Copy link
Copy Markdown
Member

@murad-hubib, done. I created the release too. Thank you for the quick turnaround.

@murad-hubib
murad-hubib deleted the murad/12189-feedback-close-btn branch January 13, 2020 13:01
@murad-hubib

Copy link
Copy Markdown
Contributor Author

Thanks @Agrendalath for prompt response

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.

2 participants