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

Add tests and styling for betterbuttons port #744

Merged

Conversation

bergice
Copy link
Contributor

@bergice bergice commented Nov 5, 2018

Fixes #436

@bergice bergice requested a review from unclecheese November 5, 2018 09:26
@bergice bergice force-pushed the pulls/1/436-port-betterbuttons branch 2 times, most recently from 44df9de to 410956d Compare November 7, 2018 05:19
@bergice bergice force-pushed the pulls/1/436-port-betterbuttons branch from 410956d to ef51328 Compare November 12, 2018 23:12
Copy link

@unclecheese unclecheese left a comment

Choose a reason for hiding this comment

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

Really nice job on the Behat tests. Thank you!

Just a quick point on the javascript. As you've changed it, it has the potential to break other people's code, and I'm pretty sure it's not necessary.

client/src/legacy/LeftAndMain.js Show resolved Hide resolved
@bergice bergice force-pushed the pulls/1/436-port-betterbuttons branch 2 times, most recently from 35981ab to a84c765 Compare November 14, 2018 04:07
@ScopeyNZ
Copy link
Contributor

ScopeyNZ commented Nov 19, 2018

How do we feel about getting some instant tooltips on these buttons? Not everyone's familiar with betterbuttons 🙂

Looks beautiful but it does look like it differs a bit from the designs

@ScopeyNZ ScopeyNZ requested a review from clarkepaul November 19, 2018 03:53
@sachajudd
Copy link
Contributor

sachajudd commented Nov 19, 2018

Hey, I've had a quick look at this and noticed it doesn't quite match the designs.

Current:
image

  • Missing background behind the arrows.

A few options for the icon:

  • The icon could be done in CSS since the icon is currently not in the icon set.
  • We could use the close icon and rotate it 45 degrees.
  • Use font-icon-plus-circled and change the icon background to green.
  • Leave as is and replace the icon when we update the icon set.

Styleguide (click the preview link): https://projects.invisionapp.com/dsm/silver-stripe/silver-stripe/asset/components/5b5e610563c7ad0012bc21fb

@sachajudd
Copy link
Contributor

Great idea @ScopeyNZ 🙂 see updated Styleguide for tooltips.

@bergice
Copy link
Contributor Author

bergice commented Nov 19, 2018

@sachajudd

Missing background behind the arrows.

This is using our current button styling, we may want to discuss if we want this changed across all buttons to be consistent and possibly added to a separate issue, but I'm leaving this as is for now.

We could use the close icon and rotate it 45 degrees.

I tried this, and it works great, but I'm also worried about consistency here. This button is equivalent to the one shown on on the grid field so ideally they should be matching. I do think this new design looks slightly better, so maybe we can change the gridfield one to match the new one.

@ScopeyNZ

How do we feel about getting some instant tooltips on these buttons? Not everyone's familiar with betterbuttons 🙂

I'll see if I can add this to FormAction's.

@sachajudd
Copy link
Contributor

We have designed better buttons to work within the CMS with these new custom styles. We haven’t added any buttons without text yet in the CMS so because it's a new pattern we'd rather it look more cleaner rather than consistent. We hope to start using these styles throughout the CMS more often in the future.

@bergice bergice force-pushed the pulls/1/436-port-betterbuttons branch from a84c765 to 5f52b60 Compare November 19, 2018 21:41
@unclecheese unclecheese merged commit 7a9920b into silverstripe:1 Nov 22, 2018
@unclecheese unclecheese deleted the pulls/1/436-port-betterbuttons branch November 22, 2018 23:40
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.

4 participants