changed styling of button-bar #376

endurance21 · 2020-02-20T14:24:04Z

Problem

https://tickets.metabrainz.org/projects/BB/issues/BB-421?filter=allopenissues

Solution

before ::::

after::::

Areas of Impact

bookbrainz-site/src/client/entity-editor/button-bar/alias-button.js
bookbrainz-site/src/client/entity-editor/button-bar/button-bar.js
bookbrainz-site/src/client/entity-editor/button-bar/identifier-button.js

coveralls · 2020-02-20T14:28:09Z

Coverage increased (+0.01%) to 43.563% when pulling b1efcc2 on endurance21:master into fe5f226 on bookbrainz:master.

prabalsingh24

Hi @endurance21
I see you've made changes in your master repository. It would've been better if you created a new branch and made changes in that. Doing so you can work on multiple issues at a time.
The 3 buttons should be in a single row unless the screen size is very small ( mobile). Button on top on each other do not look very good.
Also I think you should add icon in the buttons. It will be like this.
<FontAwesomeIcon icon="plus"/> <span> Add relationship</span>
Have a look here: c9f4814

endurance21 · 2020-02-21T00:56:35Z

@prabalsingh24
Thanks for the branch take 😅, I will keep in mind from next time.

Buttons are indeed in same row, I just presented the mobile view. Now, in mobile view buttons will have to be like that only, i.e in different rows.. In Web view it is in same row..

And as far as icon is concerned, I was thinking of doing that too. Thanks for the reference to your fontAwesome icons.

I am doing the icon changes right there.

prabalsingh24 · 2020-02-21T00:58:55Z

Oh sorry about that. I didn't test in my local server. I just saw the picture. It looked like web browser :p.
You go ahead and add icons here. :)

endurance21 · 2020-02-21T01:39:10Z

Oh sorry about that. I didn't test in my local server. I just saw the picture. It looked like web browser :p.
You go ahead and add icons here. :)

added them

endurance21 · 2020-02-21T01:39:47Z

@prabalsingh24 is it okay now?

prabalsingh24

Your code failed the eslint tests.
https://travis-ci.org/bookbrainz/bookbrainz-site/jobs/653265678?utm_medium=notification&utm_source=github_status
you can see the errors here.

prabalsingh24 · 2020-02-21T01:57:01Z

Also rewriting commits was not needed, and you should avoid using git push --force. You're working alone in this so it's fine but when working in a team it's better to have an extra commit than using --force
https://stackoverflow.com/questions/18357511/git-remove-committed-file-after-push/22041320#22041320
while potentially running into a conflict with your colleague who may have pulled it already, and who will grow grey hair and lose lots of time trying to reconcile his local branch head with the central one:

endurance21 · 2020-02-21T01:58:12Z

@prabalsingh24
thanks for the report ,
i have modified the pr and is passing the eslint tests now, i actually forgot some extra trailing lines and ordering of imports.

endurance21 · 2020-02-21T02:00:10Z

Also rewriting commits was not needed, and you should avoid using git push --force. You're working alone in this so it's fine but when working in a team it's better to have an extra commit than using --force
https://stackoverflow.com/questions/18357511/git-remove-committed-file-after-push/22041320#22041320
while potentially running into a conflict with your colleague who may have pulled it already, and who will grow grey hair and lose lots of time trying to reconcile his local branch head with the central one:

ya , it's a good practise,
thanks .

prabalsingh24 · 2020-02-21T09:00:53Z

Umm no tests failed again :P

@prabalsingh24
thanks for the report ,
i have modified the pr and is passing the eslint tests now, i actually forgot some extra trailing lines and ordering of imports.

prabalsingh24 · 2020-02-21T11:14:29Z

Tests are still failing :P

endurance21 · 2020-02-21T13:40:59Z

updated !

endurance21 · 2020-02-22T06:54:15Z

@MonkeyDo
Have a look over this. Thanks.

prabalsingh24 · 2020-02-22T09:07:46Z

@MonkeyDo
Have a look over this. Thanks.

I remember that he said he's little busy this week. I am sure he'll review this when he gets free, even one of my PR's review is pending XD

endurance21 · 2020-02-22T09:48:27Z

@prabalsingh24
Ya i remember that.
I just marked the final say of our conversation, by that comment... 😅😅

endurance21 · 2020-02-22T18:23:59Z

i have also changed the button on add edition page
"ADD PHYSICAL DATA" button

prabalsingh24 · 2020-02-22T18:28:05Z

i have also changed the button on add edition page
"ADD PHYSICAL DATA" button

It would've been better if you made a new commit for this. Easy to review :)

endurance21 · 2020-02-22T19:25:03Z

@prabalsingh24
Actually i thought, changing styles of button is same type of work, either done on button bars or on edition page.
So why should I explicitly mention that.
But ya for the sake of understanding, your say is appreciable...

MonkeyDo

This is a good start, I agree that it was not clear that those were buttons, and it needs to be changed.
I have a couple of issues however that I don't have a solution ready for.

The first one is about how the buttons are displayed at certain sizes: the bootstrap class col-md-4 divides the row in three equal parts, but the button may not fit in that column, and will take more space, creating an undesirable uneven spacing .
Here's for example what that would look like on an iPad:

The second one concerns the indicator when an alias or identifier is incorrect.
You still end up with a red "x" in front of the button text, but It is not as visible as before.
The goal is to indicate clearly to the user that there is a problem there they need to rectify.
Here's what I'm talking about (same for the add alias button):

I'm not sure what's the best way to solve either of these issues, but I'm sure you can find good solutions :)

endurance21 · 2020-02-22T22:32:08Z

@MonkeyDo
thanks for reviewing .

your first concern about fitness of buttons
my say::::: this is only the problem in devices other than mobile devices . so i only need to make
the solution for devices other than mobile . i am working on it.

2.your second concern about icons to display need of rectification .
my say::::::for this i will just make the icon element dynamic , and implement a state to entitty-
editor component that is just a bool that will store if rectification needed or not .
as per the input , the state of entity-editor component will change (with setstate) and so
as the dynamic icon will be update there .

does that sounds good?

MonkeyDo · 2020-02-22T23:21:09Z

After thinking about it for a bit, I think there are some considerations that will solve a few issues:

I think the disambiguation should always be there, meaning we won't need a button
The identifiers should be presented on the page without having to open a popup. There's a ticket open for that: https://tickets.metabrainz.org/browse/BB-343
With 2. implemented, perhaps there would be a good way to display aliases and allow to add more aliases directly on the page without a popup editor…

I think those should be done as separate PRs, but they will definitely change things for this current PR.

Regarding your proposal for the rectification icon, the issue is not only the icon itself, but more so the fact that it is not very visible. Changing the "+" into an "x" will not solve that issue, and maybe even make it even less visible.
Would you need to turn the whole button orange or red, Or some other solution?

endurance21 · 2020-02-23T15:46:36Z

@MonkeyDo
That means I should,

Remove all pop-up feature?.
2.make available all inputs(add disambiguation, add aliases) on same page
On which particular entity editor is present.
That will solve BB343 as well.
Am I getting it right?
And should I make another PR. For all these changes
And what will happen to current pr?.

endurance21 · 2020-02-24T17:30:17Z

sorry , i have deleted some commits by mistake from master , that's why pr was closed.
now it is restored !

endurance21 · 2020-02-25T09:37:40Z

@MonkeyDo
i have done part 1,
deleted disambiguation button , and added disambiguation field by default ,
should i push in this PR or i havet to make another PR for that ,cause the title of this PR is not matching the work i have done.

please respond , as my semester exams are going on this week , i have to find the time to work on it , tell me should i make another Pr for this or should i make changes in this Pr only,
also i have to complete part 2 once i am done with part 1 as you suggested .

MonkeyDo · 2020-03-02T18:54:32Z

Good job on that first PR !

You are correct about the rest: the goal is to move the alias and identifiers popups content into the page.
It might be helpful to do a quick mockup beforehand, to see how everything will fit on the page

endurance21 · 2020-03-03T15:18:33Z

@MonkeyDo
I have already made the design, I was waiting for your responses..
Here it is
https://xd.adobe.com/view/36b49154-f1ed-4803-4a44-c8897dcca694-4085/

endurance21 · 2020-03-07T13:20:41Z

hey @MonkeyDo !
what you say about my design ?
https://xd.adobe.com/view/36b49154-f1ed-4803-4a44-c8897dcca694-4085/
should i start building them ?

prabalsingh24 · 2020-03-07T14:16:14Z

Don't you think it's better if you just add those two - alias editor and identifier editor - in the current design. I mean what you suggested would involve complete redesign of the website, which is alot of work!?
In any case have a look at this too.
https://tickets.metabrainz.org/projects/BB/issues/BB-236

endurance21 · 2020-03-07T16:32:22Z

@prabalsingh24
I appreciate your concern about a lot of work! 😅
But what matters here is what we give to the end user.
And for me, making that happen is the goal I decided at the first place!

And for the record, I may include this in my proposal for gsoc, if it is needed to be done.!!!

MonkeyDo · 2020-03-12T14:43:22Z

@endurance21 At this point I don't think we should do an entire overhaul of the design. That's not a priority, and would require a lot more preparation, discussion, designs, etc.

The mockups I'm more interested in should be based on the existing layout, but modified as we've discussed to move the alias and identifier editors into the main body of the page.

endurance21 · 2020-03-12T17:56:05Z

@MonkeyDo
Okay! I get it
I will keep the existing layout!
One more thing!
As I have removed the disambiguation button, should I know change the buttons too?. I mean for the purpose this current pr was made?.
Will have to make another PR?

MonkeyDo · 2020-03-12T17:58:44Z

I think it would be better to do a PR each for the alias editor and for the identifier editor, and abandon this PR, as I feel the problem would be solved another way.

MonkeyDo · 2020-04-06T13:09:55Z

I am closing this in favor of creating two separate PRs for moving both aliases and identifiers in the main flow of the page, as described in ticket https://tickets.metabrainz.org/browse/BB-343

endurance21 requested a review from MonkeyDo February 20, 2020 14:24

endurance21 requested a review from leftmostcat February 20, 2020 23:19

prabalsingh24 suggested changes Feb 21, 2020

View reviewed changes

endurance21 force-pushed the master branch from fad5a8f to 098e18d Compare February 21, 2020 01:36

prabalsingh24 suggested changes Feb 21, 2020

View reviewed changes

endurance21 force-pushed the master branch from 098e18d to 6a1a508 Compare February 21, 2020 01:55

endurance21 force-pushed the master branch from 6a1a508 to 5f4317c Compare February 21, 2020 12:22

endurance21 force-pushed the master branch from 5f4317c to f380f6f Compare February 22, 2020 18:21

MonkeyDo requested changes Feb 22, 2020

View reviewed changes

endurance21 force-pushed the master branch from 7af1857 to cbda1b4 Compare February 23, 2020 14:31

endurance21 closed this Feb 24, 2020

endurance21 force-pushed the master branch from cbda1b4 to fe7961e Compare February 24, 2020 16:50

changed styling of button-bar

5754ffc

endurance21 reopened this Feb 24, 2020

endurance21 mentioned this pull request Feb 25, 2020

removed disambiguation button and added disambiguation input as a def… #381

Merged

Merge branch 'master' into master

b1efcc2

MonkeyDo closed this Apr 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

changed styling of button-bar #376

changed styling of button-bar #376

endurance21 commented Feb 20, 2020 •

edited

coveralls commented Feb 20, 2020 •

edited

prabalsingh24 left a comment

endurance21 commented Feb 21, 2020

prabalsingh24 commented Feb 21, 2020

endurance21 commented Feb 21, 2020

endurance21 commented Feb 21, 2020

prabalsingh24 left a comment

prabalsingh24 commented Feb 21, 2020

endurance21 commented Feb 21, 2020

endurance21 commented Feb 21, 2020

prabalsingh24 commented Feb 21, 2020 •

edited

prabalsingh24 commented Feb 21, 2020

endurance21 commented Feb 21, 2020

endurance21 commented Feb 22, 2020

prabalsingh24 commented Feb 22, 2020

endurance21 commented Feb 22, 2020

endurance21 commented Feb 22, 2020

prabalsingh24 commented Feb 22, 2020

endurance21 commented Feb 22, 2020

MonkeyDo left a comment

endurance21 commented Feb 22, 2020 •

edited

MonkeyDo commented Feb 22, 2020 •

edited

endurance21 commented Feb 23, 2020

endurance21 commented Feb 24, 2020 •

edited

endurance21 commented Feb 25, 2020 •

edited

MonkeyDo commented Mar 2, 2020

endurance21 commented Mar 3, 2020

endurance21 commented Mar 7, 2020

prabalsingh24 commented Mar 7, 2020

endurance21 commented Mar 7, 2020 •

edited

MonkeyDo commented Mar 12, 2020

endurance21 commented Mar 12, 2020

MonkeyDo commented Mar 12, 2020

MonkeyDo commented Apr 6, 2020

changed styling of button-bar #376

changed styling of button-bar #376

Conversation

endurance21 commented Feb 20, 2020 • edited

Problem

Solution

Areas of Impact

coveralls commented Feb 20, 2020 • edited

prabalsingh24 left a comment

Choose a reason for hiding this comment

endurance21 commented Feb 21, 2020

prabalsingh24 commented Feb 21, 2020

endurance21 commented Feb 21, 2020

endurance21 commented Feb 21, 2020

prabalsingh24 left a comment

Choose a reason for hiding this comment

prabalsingh24 commented Feb 21, 2020

endurance21 commented Feb 21, 2020

endurance21 commented Feb 21, 2020

prabalsingh24 commented Feb 21, 2020 • edited

prabalsingh24 commented Feb 21, 2020

endurance21 commented Feb 21, 2020

endurance21 commented Feb 22, 2020

prabalsingh24 commented Feb 22, 2020

endurance21 commented Feb 22, 2020

endurance21 commented Feb 22, 2020

prabalsingh24 commented Feb 22, 2020

endurance21 commented Feb 22, 2020

MonkeyDo left a comment

Choose a reason for hiding this comment

endurance21 commented Feb 22, 2020 • edited

MonkeyDo commented Feb 22, 2020 • edited

endurance21 commented Feb 23, 2020

endurance21 commented Feb 24, 2020 • edited

endurance21 commented Feb 25, 2020 • edited

MonkeyDo commented Mar 2, 2020

endurance21 commented Mar 3, 2020

endurance21 commented Mar 7, 2020

prabalsingh24 commented Mar 7, 2020

endurance21 commented Mar 7, 2020 • edited

MonkeyDo commented Mar 12, 2020

endurance21 commented Mar 12, 2020

MonkeyDo commented Mar 12, 2020

MonkeyDo commented Apr 6, 2020

endurance21 commented Feb 20, 2020 •

edited

coveralls commented Feb 20, 2020 •

edited

prabalsingh24 commented Feb 21, 2020 •

edited

endurance21 commented Feb 22, 2020 •

edited

MonkeyDo commented Feb 22, 2020 •

edited

endurance21 commented Feb 24, 2020 •

edited

endurance21 commented Feb 25, 2020 •

edited

endurance21 commented Mar 7, 2020 •

edited