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

NEW Add VersionedBadge for displaying versioning states in the CMS #861

Conversation

robbieaverill
Copy link
Contributor

@robbieaverill robbieaverill commented Apr 17, 2019

Replacement PR for #817 which adds a new wrapper component for versioning state badges, instead of adding more props to the Badge component.

Originally raised in silverstripe/silverstripe-campaign-admin#77 since we're displaying the wrong badges for versioning in campaign admin.

Parent issue

Related PRs

@maxime-rainville maxime-rainville force-pushed the pulls/1.4/versioned-state-badge branch from d3a9ffe to 7a2ffbf Compare August 29, 2019 01:41
@maxime-rainville maxime-rainville force-pushed the pulls/1.4/versioned-state-badge branch from 7a2ffbf to ef4b191 Compare August 29, 2019 01:43
@maxime-rainville
Copy link
Contributor

I've rebased, remove the message (since this can be inferred from the status), added an archived status and functionalise the component.

Here's what the pattern lib looks like.
image

@silverstripeux Can you let us know if you are happy with those badges? For context, this component will eventually be use everywhere we display a stage label.

Copy link
Contributor

@maxime-rainville maxime-rainville left a comment

Choose a reason for hiding this comment

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

I'm happy but please wait for design review before merge.

Copy link
Contributor Author

@robbieaverill robbieaverill left a comment

Choose a reason for hiding this comment

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

The only thing with removing the message and inferring it from the status is we'll need to still allow people to add their own states I guess, Fluent for example adds its own to the site tree

@sachajudd
Copy link
Contributor

I think you might be using the incorrect variable sheet. I see you're using _bootstrap-variables rather than using the "Extra state colours" in _variables.scss. Which include the correct state-draft and state-modified colours. Not sure if this is the same with the live and archived colours but you might want to check incase 🙂

@maxime-rainville
Copy link
Contributor

The only thing with removing the message and inferring it from the status is we'll need to still allow people to add their own states I guess, Fluent for example adds its own to the site tree

Shouldn't other modules be using the Badge component directly instead?

@sachajudd
Copy link
Contributor

sachajudd commented Aug 29, 2019

Just a side note from Max's screenshot the type face almost looks a little off and the letter spacing. Not sure what's going on there (would be good to compare) but I understand once it's in the pattern library we can tweak it.

Pattern lib:
image

Demo:
image
E.g the 'D' in MODIFIED looks quite spaced in the first screenshot. What browser were you using @maxime-rainville ?

@maxime-rainville
Copy link
Contributor

E.g the 'D' in MODIFIED looks quite spaced in the first screenshot. What browser were you using

I've got the same gap in the CMS tree. It might just be that I'm looking at it on Ubuntu and you are looking at it on a Mac. Mine is falling back to Arial because I don't have Helvetica Neue or Helvetica.

Copy link
Contributor Author

@robbieaverill robbieaverill left a comment

Choose a reason for hiding this comment

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

Nice! Can't approve my own PR but I can say this instead

@robbieaverill
Copy link
Contributor Author

We need to follow up on @sachajudd's comments first, and remove the use of the old variable in Badge.scss (doing this now)

@robbieaverill
Copy link
Contributor Author

@sachajudd this PR makes $state-archived: $gray-700. Originally .status-archived { color: $text-muted; } evaluates $text-muted to $gray-600. Which should be used?

@sachajudd
Copy link
Contributor

sachajudd commented Aug 30, 2019

Yeah lets leave this to a separate PR as I've noticed all of the colours are a bit out. Stick with $gray-700 for now and I'll take a look next week. Happy for the PR to be merged 👍

@robbieaverill robbieaverill merged commit 28c6e21 into silverstripe:1 Aug 30, 2019
@robbieaverill robbieaverill deleted the pulls/1.4/versioned-state-badge branch August 30, 2019 03:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants