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.

Sign up for GitHub

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

Jump to bottom

Replace usage of text-secondary and text-primary for coloring elements that present actions and information #6154

Merged
merged 2 commits into from Nov 2, 2018
Merged

Replace usage of text-secondary and text-primary for coloring elements that present actions and information #6154

merged 2 commits into from Nov 2, 2018

Conversation

bgeuken
Copy link
Member

@bgeuken bgeuken commented Oct 30, 2018 •
edited by dmarcoux

This PR replaces usage of

  • text-primary by text-success for elements that signaling success, like build state, as well as for the enabled flags icon
  • and text-secondary by text-info for elements that provide information

Fixes of the cases of #6091

Screenshot (so you can see the differences in the colors):

  • Right side, status messages: text-success (green) and text-info (blue)
  • Bottom, buttons: text-primary (green) and text-secondary (blue)
    diff-colors

@bgeuken bgeuken added Frontend Things related to the OBS RoR app Bootstrap 🚀 Bootstrap migration labels Oct 30, 2018
@bgeuken
Copy link
Member Author

bgeuken commented Oct 30, 2018 •
edited

I am going to add some screenshots tomorrow.

@dmarcoux
Copy link
Contributor

It's not a blocker for this PR, but we need to update this too in obs-patterns. Some icons there are using text-primary and text-secondary when they should not.

@dmarcoux dmarcoux added the review-app Apply this label if you want a review app started label Nov 1, 2018
@obs-bot
Copy link
Collaborator

obs-bot commented Nov 1, 2018

Review app will appear here: http://obs-reviewlab.opensuse.org/bgeuken-update_colors_in_webui2

@dmarcoux
Copy link
Contributor

dmarcoux commented Nov 2, 2018

I added the screenshot

@dmarcoux
Copy link
Contributor

dmarcoux commented Nov 2, 2018

I don't have an issue with having 2 greens and 2 blues. They convey different meanings depending on the class we use.

@bgeuken @dmarcoux
Replace inproper usage of text-primary
9438e0b 
As @hellcp pointed out the actual primary color might
change over time. Thus we drop the usage of text-primary where we
want greeb colooring.

Since we decided that we don't want to define additional color
variables we use the text-success when we want green coloring, eg.
succeeded build states or enabled flags.

Part of openSUSE#6091
@bgeuken @dmarcoux
Use text-info for coloring elements that inform users
d76874c 
We have beeeen using the secondory color before. Which kinda worked
because it's blue. But as soon as we change the secondary color
we would have to update this.

Kudos goes to @hellcp for pointing this out.

Part of openSUSE#6091
@dmarcoux
Copy link
Contributor

dmarcoux commented Nov 2, 2018

I just rebased on master so Hakiri doesn't fail for outdated vulnerabilities (which are fixed in master)

@dmarcoux dmarcoux mentioned this pull request Nov 2, 2018
Merged
@dmarcoux
Copy link
Contributor

dmarcoux commented Nov 2, 2018 •
edited

The follow-up PR for obs-patterns is openSUSE/obs-patterns#39

@bgeuken
Copy link
Member Author

bgeuken commented Nov 2, 2018

The CircleCi tests passed in my CircleCI "branch". It's just that the result was not updated here.

Thus merging. (Though we should investigate why this happens more frequently now)

@bgeuken bgeuken merged commit eed4c04 into openSUSE:master Nov 2, 2018
@bgeuken bgeuken deleted the update_colors_in_webui2 branch November 2, 2018 12:31
@coolo
Copy link
Member

coolo commented Nov 2, 2018

you need to unfollow your repository - if you follow it, circle does not create the required pr_X workflow.

This circleci is super-not-convincing, not sure we can go back to webhooks though

@coolo
Copy link
Member

coolo commented Nov 2, 2018

the circleci app I mean

@bgeuken bgeuken mentioned this pull request Nov 12, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DavidKang DavidKang DavidKang approved these changes

@Ana06 Ana06 Ana06 approved these changes

Assignees
No one assigned
Labels
Bootstrap 🚀 Bootstrap migration Frontend Things related to the OBS RoR app review-app Apply this label if you want a review app started
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@bgeuken @dmarcoux @obs-bot @coolo @DavidKang @Ana06