Closed Bug 1182617 Opened 6 years ago Closed 6 years ago

"Please select a star rating" should not be showed after selecting a star rating

Categories

(Marketplace Graveyard :: Consumer Pages, defect, P5)

Avenir
defect

Tracking

(Not tracked)

RESOLVED INVALID

People

(Reporter: atiqueahmedziad, Unassigned, Mentored)

References

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 file)

Attached image bug.png
STP :
1. Go to any app detail page with your mobile.
2. Select "Write a review" 
3. Give a star rating

Actual result: 
after selecting a star rating,  there still says "Please select a star rating"

Expected result: 
after selecting a star rating,  the string "Please select a star rating" should be hidden.
Assignee: nobody → softfilebd
Assignee: softfilebd → nobody
Mentor: softfilebd
Whiteboard: [good first bug][lang=js]
Have submitted PR
https://github.com/mozilla/fireplace/pull/1383
Kindly review
See Also: → 1183067
Going to get some direction from @pwalm on this. It definitely seems a bit weird that it is there, perhaps the wording should change but it doesn't seem that weird to me.

@pwalm what should be done here?

I think we should make sure we don't change the height of the page when a star gets selected, if we hide the text dynamically it should be with something like: `color: transparent` or removing the text instead of hiding the element.
Flags: needinfo?(pwalmsley)
So I amended it with visibility: hidden, will that suffice?
(In reply to Trishul from comment #1)
> Have submitted PR
> https://github.com/mozilla/fireplace/pull/1383
> Kindly review

I wouldn't like to go with this, since I am seeing it only fixing the page - https://marketplace-dev.allizom.org/app/delicious-pierogi-7/ratings/add 
Rather we need to fix both review popup in desktop and `ratings/add` page. So, I will suggest you to work with this onclick function: https://github.com/mozilla/fireplace/blob/master/src/media/js/ratingwidget.js#L49-L59  

(In reply to Mark Striemer [:mstriemer] from comment #2)
> Going to get some direction from @pwalm on this. It definitely seems a bit
> weird that it is there, perhaps the wording should change but it doesn't
> seem that weird to me.
> 
> @pwalm what should be done here?
> 
> I think we should make sure we don't change the height of the page when a
> star gets selected, if we hide the text dynamically it should be with
> something like: `color: transparent` or removing the text instead of hiding
> the element.

If we use `visibility: hidden` it gives a blank white space removing the text. And if we use `display: none` that white cuts off. 

How it looks with `visibility: hidden` -
https://www.dropbox.com/s/ynv8uy3zfjx8koh/Screenshot%20from%202015-07-14%2000%3A34%3A51.png?dl=0
https://www.dropbox.com/s/isz850tl493nxv6/Screen%20Shot%202015-07-03%20at%2000.34.39.png?dl=0

How it looks with `display: none` -
https://www.dropbox.com/s/or9nggxn21igicx/Screenshot%20from%202015-07-14%2000%3A36%3A10.png?dl=0
https://www.dropbox.com/s/t05ndc2nbds5f5z/Screen%20Shot%202015-07-03%20at%2000.36.15.png?dl=0

For me, I think the white is unnecessary, but leaving the decision to Phil & Mark :)
yes the changes can be made in the widget file itself, just when Phil/Mark reply, I will update the PR
Priority: -- → P5
This has been addressed in Phase 1 of MOW, a fix is coming! :)
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(pwalmsley)
Resolution: --- → FIXED
Looks like we don't want to make this change. Thank you for the patches, once the design settles this will likely be another [good first bug].
Resolution: FIXED → INVALID
You need to log in before you can comment on or make changes to this bug.