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

topics tracklist #65

Merged

Conversation

carlynlee
Copy link
Collaborator

@carlynlee carlynlee commented Mar 5, 2024

relates to: #63

shortens the tracklist, Match color of tracklisting with schedule

result of these changes:
Screenshot 2024-03-04 at 3 31 16 PM

Copy link
Member

@kylerisse kylerisse left a comment

Choose a reason for hiding this comment

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

Thanks @carlynlee . Appreciate you updating the color schemes for new and renamed tracks. I just have one concern noted in line. Generally not a fan of special characters in map keys and unsure how that might affect css.

server/style.css Outdated
.Youth {
background: #098C09 !important;
color: #fff !important;
.FOSS@HOME {
Copy link
Member

Choose a reason for hiding this comment

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

I'm concerned the use of @ in the selector name might not be valid. Can you confirm @carlynlee @sarcasticadmin ?

Copy link
Member

Choose a reason for hiding this comment

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

thats a good question, let me do some digging

Copy link
Member

Choose a reason for hiding this comment

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

From the example that @carlynlee sent over while work through this it does look like FLOSS@HOME is not the expected color. Its grey (default) in this screenshot:

Screenshot 2024-03-04 at 3 31 16 PM

I believe we just need to escape the @:

.FOSS\@HOME {

If you want a quick example in https://jsfiddle.net/ :

html

<div class="container">
  <div class="one">
    <p>One</p>
  </div>
  <div class="floss@home">
    <p>floss@home</p> 
  </div>
  <div class="floss@broken">
    <p>floss@broken</p>
  </div>
</div>

css:

/* For each class, set some colors */
.one {
  background-color: cornflowerblue;
}
.floss\@home {
  color: black;
  background-color: #8ADA20;
}
.floss@broken {
  color: black;
  background-color: #8ADA20;
}

Results in:
ss-202403051709612052

Copy link
Member

@kylerisse kylerisse Mar 5, 2024

Choose a reason for hiding this comment

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

I believe the selector name is the map key value. My suggestion is to just omit the @ from the key here

"FOSS@HOME" => "FOSS @ HOME",
, making it FOSSHOME and then match the CSS selector to FOSSHOME. Same pattern as how spaces are treated. Probably cleaner than escaping.

Copy link
Member

Choose a reason for hiding this comment

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

that makes sense, definitely safer 👍

Copy link
Member

@sarcasticadmin sarcasticadmin left a comment

Choose a reason for hiding this comment

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

@carlynlee topics are looking good and awesome job getting these colors to align with the main schedule ! This will really go a along way to make it easy for the Scale attendees to identify their designed tracks since it will match the program.

Per Kyles suggestion, I think we just need to make that small tweak in the css (see my comment inline below) and then it should be good. Would you mind testing that out?

server/style.css Outdated
.Youth {
background: #098C09 !important;
color: #fff !important;
.FOSS@HOME {
Copy link
Member

Choose a reason for hiding this comment

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

From the example that @carlynlee sent over while work through this it does look like FLOSS@HOME is not the expected color. Its grey (default) in this screenshot:

Screenshot 2024-03-04 at 3 31 16 PM

I believe we just need to escape the @:

.FOSS\@HOME {

If you want a quick example in https://jsfiddle.net/ :

html

<div class="container">
  <div class="one">
    <p>One</p>
  </div>
  <div class="floss@home">
    <p>floss@home</p> 
  </div>
  <div class="floss@broken">
    <p>floss@broken</p>
  </div>
</div>

css:

/* For each class, set some colors */
.one {
  background-color: cornflowerblue;
}
.floss\@home {
  color: black;
  background-color: #8ADA20;
}
.floss@broken {
  color: black;
  background-color: #8ADA20;
}

Results in:
ss-202403051709612052

README.md Outdated
@@ -86,6 +86,7 @@ There is a bit of manual effort necessary from year to year. These tasks include
* update `$shorten_topics` in `scroll.php` to reflect the updated track list, matching exactly the keys to what is being supplied by the xml from drupal minus spaces
* update `$shorten_topics` in `room.php` to match `scroll.php`
* create a style for each of the keys in `$shorten_topics` in `style.css` matching it to the public site
* add colors for topics/tracks in `style.css`
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding this to the list!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I removed the special char @ from two files:

https://github.com/carlynlee/scale-signs/blob/shorten_topics_tracklist/server/scroll.php#L50

https://github.com/carlynlee/scale-signs/blob/shorten_topics_tracklist/server/style.css#L561

I rebuilt the image and tried to view the track listing again but I'm unsure why the colors aren't applied:
Screenshot 2024-03-04 at 3 31 16 PM

Copy link
Member

Choose a reason for hiding this comment

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

did you rebuild the container? It looks like its still the old name Foss @ Home from your screenshot (which it should be FOSS HOME from your diff)

BTW keep the @ on the right side of the assignment. So the entry in scroll.php would look like:

"FOSSHOME"	=>	 "FOSS @ HOME",

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep, I ran docker-compose build --no-cache and then docker-compose up -d

$node->{'Topic'} = preg_replace('/\s+/', '', $node->{'Topic'});
$node->{'Topic'} = preg_replace('/\&/', 'and', $node->{'Topic'});
$node->{'Topic'} = preg_replace('/\@/', '', $node->{'Topic'});
Copy link
Member

Choose a reason for hiding this comment

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

@carlynlee did a little digging, turns out we need to replace the @ in the original Topic name we get from the XML as well. Its due to the lookup that takes place here:

if ( array_key_exists($topic, $shorten_topics) ) {

It all looks good for me now 🙂
ss-202403061709684396

I pushed these changes directly on your branch so just pull them down with: git pull -r origin shorten_topics_tracklist and you should these my additions

Copy link
Collaborator Author

@carlynlee carlynlee Mar 6, 2024

Choose a reason for hiding this comment

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

Thanks @sarcasticadmin ! Seems like the this pull req is ready to merge. Your changes and it seem to work on my end too:
Screenshot 2024-03-06 at 9 43 21 AM

@sarcasticadmin sarcasticadmin force-pushed the shorten_topics_tracklist branch from 211fa8b to aa62caf Compare March 6, 2024 18:11
Copy link
Member

@sarcasticadmin sarcasticadmin left a comment

Choose a reason for hiding this comment

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

@carlynlee this is fantastic, the signs server is really coming along 🎉

I went ahead and rebased these commits so the history was more straightforward prior to use merging it into master

@sarcasticadmin sarcasticadmin force-pushed the shorten_topics_tracklist branch from aa62caf to b158167 Compare March 6, 2024 18:15
@sarcasticadmin sarcasticadmin merged commit dc7447f into socallinuxexpo:master Mar 6, 2024
1 check passed
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.

3 participants