Within Olivero, the progress indicator bar percentage label doesn't look quite right. You can see this using the CD_Tools module and at https://tugboat-aqrmztryfqsezpvnghut1cszck2wwasr.tugboat.qa/progress

Issue fork drupal-3277557

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mherchel created an issue. See original summary.

rootwork’s picture

Issue tags: +Novice, +Portland2022

Tagging as novice for Portland contrib

galactus86’s picture

I am going to work on this.

rootwork’s picture

Turned out testing this had some dependencies :)

1. We got DrupalPod installed.
2. We figured out how to add an SSH key in DrupalPod.
3. We figured out how to get a GitHub token so we could `composer require` https://github.com/zolhorvath/cd_tools from GH, in DrupalPod. CD Tools is part of Olivero testing (mentioned in IS).
4. We read How to contribute to Olivero.
4. We figured out how to install yarn and compile postCSS into CSS.
5. We read Olivero's (Claro's) CSS coding standards.

The CSS fix is straightforward, but it took a bit to get to the point where we could make it. Lots of learning on this little issue!

galactus86’s picture

#5 is my first attempt at a merge request. The issue itself is a simple CSS change. Please advise if my MR is incorrect or any tips. thx

mherchel’s picture

Status: Active » Needs work

@galactus86

Thanks for the patch! There are a couple issues.

  1. The CI says "Custom commands failed" because the compiled output doesn't match what's in the MR. After you do the other fixes, you need to make sure it's up to date by doing yarn build:css or yarn watch:css (and then make the changes)
  2. Olivero also supports RTL languages, so we need to make sure that we support that with this too. To do this, you'll want to use CSS logical properties (in this case margin-inline-start), which will be compiled by PostCSS into code that all supported browsers support.
  3. I'd personally add some more spacing. Maybe 1rem, but  🤷‍♂️
mherchel’s picture

4. Also, the class name in the CSS isn't preceded by a period (.).

galactus86’s picture

Adjustments made @mherchel - I ran both watch, build before this latest change and the again after. Just getting the hand of these tools. thanks for the quick response.

mherchel’s picture

Status: Needs work » Needs review

Looking a lot better! Don't forget to set the status afterwards!

Can't pull down and review now, but it looks good. Will check it out tomorrow.

mherchel’s picture

Status: Needs review » Needs work

@galactus86 Looks like the patch is created against 9.5.x (I didn't even know that branch was open). Since you created the MR, you're the only one that can update it. Can you point it against 9.4.x please?

galactus86’s picture

Status: Needs work » Needs review

Changed the target branch per @mherchel.

mherchel’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
103.42 KB

Changes look perfect. Thanks for the work on this @galactus86 (and @rootwork)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 946d5a3 and pushed to 10.0.x. Thanks!
Committed and pushed 2d3c1013ba to 9.5.x and 34318c19ea to 9.4.x. Thanks!

I rolled the fix on top of 10.0.x for you.

  • alexpott committed 2d3c101 on 9.5.x
    Issue #3277557 by galactus86, mherchel, rootwork: Olivero: Progress...

  • alexpott committed 34318c1 on 9.4.x
    Issue #3277557 by galactus86, mherchel, rootwork: Olivero: Progress...

  • alexpott committed 946d5a3 on 10.0.x
    Issue #3277557 by galactus86, mherchel, rootwork: Olivero: Progress...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.