Feat: Add Theming with config.yml by DeepanshKhurana · Pull Request #15 · Appsilon/LogAnalyzer

DeepanshKhurana · 2024-11-22T11:58:49Z

Have you read the Contributing Guidelines?

Closes #14

Note to reviewers: This was changed before brand.yml came into the picture and works quite similarly. Still, since this is underway and has existing PRs in sequence (#17, #20), it would make sense to go forward with this and then, do a cleanup refactor to use brand.yml with minor changes, if necessary. Although, I don't see the need as of yet because the branding is not as widely present in this app, owing to its simplicity. But it can be something we decide in the future.

Description

Adds config.yml branding + Sass modifications to access it
Adds notes to README.md for the same
Adds minor fix to onClick in the app list and job list reactable tables

Loom recording with demo

Definition of Done

The change is thoroughly documented.
The CI passes (R CMD check, linter, unit tests, spelling).
Any generated files have been updated (e.g. .Rd files with roxygen2::roxygenise())

feat: add functions for coloring svg files feat: replace sass colors with css colors

jakubnowicki

Hey, this is a nice improvement 👍 It needs a little bit more polishing; please check my comments.

jakubnowicki · 2025-02-11T14:17:53Z

app/main.R

@@ -1,31 +1,17 @@
 # nolint start: box_func_import_count_linter


Please remove nolint.

Done, I've removed all nolints!

jakubnowicki · 2025-02-11T14:24:29Z

app/main.R

+    branding <- get("branding")
+
+    output$dynamic_colors <- shiny$renderUI({
+      css_content <- generate_css_variables(branding)
+      shiny$tags$style(shiny$HTML(css_content))
+    })


Instead of using renderUI, it could be simpler to just read and create the style outside of ui and server and than use it inside (something like the good old global.R).

I think this was a great suggestion, saves us the extra reactive usage when none is required. I've moved the code to a section above the ui()

jakubnowicki · 2025-02-11T14:26:45Z

app/logic/general_utils.R

+#' @return a string of CSS variables within :root {}
+#' @export
+generate_css_variables <- function(
+  config = get("branding")


Do we actually need a default here?

Correct. I've removed the default value.

jakubnowicki · 2025-02-11T14:29:01Z

app/logic/general_utils.R

+#' @param config the config file
+#' @return a string of CSS variables within :root {}
+#' @export
+generate_css_variables <- function(


Maybe it is a good moment to start adding unit tests? This function looks like an opportunity :)

I've added tests for all the general_utils functions! :D

jakubnowicki · 2025-02-11T14:33:37Z

app/logic/empty_state_utils.R

  text = "Select an application and a job to view logs",
-  image_path = "static/illustrations/empty_state.svg"
+  image_path = "static/illustrations/empty_state.svg",
+  color = NULL


This is inconsistent, as replace_svg_fill will not accept NULL.

jakubnowicki · 2025-02-11T14:34:06Z

config.yml

+        grey-text: grey
+        black-text: "#333"
+        selected-row: "#00000010"
+        primary: "#0099f9"


Maybe it would be good to move primary to the top?

Makes sense. I've moved it to the top!

jakubnowicki · 2025-02-11T14:35:57Z

README.md

    - This may mean that the Posit Connect API's response did not send proper data.
    - So far, one documented reason for this is that OAuth on Posit Connect instances may prevent the `/content` endpoint from sending app data.
+- How do I rebrand the application?
+    - You can edit the branding in the `config.yml` file. You'll find the `colors` key which will build the CSS.


I suggest expanding this with some description of what particular colors do.

Yes! I've added a few points explaining how the colors work. It should at least indicate the user what colors affect what values.

…mpty_state_ui

…refactor/simplify-state

vituri

Looks good! Later we will be able to use the new brand.yml file.

I see that all other comments by Jakub Nowicki were solved as well since his last review.

vituri · 2025-05-09T17:42:43Z

tests/testthat/test-general_utils.R

+describe("check_text_error()", {
+  it("returns TRUE for error keywords", {
+    expect_true(
+      check_text_error(
+        "Error: job failed"
+      )
+    )
+    expect_true(
+      check_text_error(
+        "Halt: job terminated"
+      )
+    )
+    expect_true(
+      check_text_error(
+        "Error: file not found"
+      )
+    )
+  })
+
+  it("returns FALSE for non-error keywords", {
+    expect_true(
+      !check_text_error(
+        "Job completed successfully"
+      )
+    )
+    expect_true(
+      !check_text_error(
+        "Processing"
+      )
+    )
+  })
+})


Is there a chance that the message is something like the following?

> check_text_error("No error found") [1] TRUE

If yes, maybe to explicit that the string start with one of the wordlist is enough, like the following:

check_text_error <- function( text, wordlist = c("halt", "err", "terminat", "not found"), ignore_case = TRUE ) { grepl( paste0("^", wordlist, collapse = "|"), text, ignore.case = ignore_case ) } check_text_error("No error found") check_text_error("Error: aaa")

Great catch! I finally took a look at this and added your code. Thanks for the suggestion.

DeepanshKhurana · 2025-10-03T08:12:54Z

@jakubnowicki I believe we should be good here and @vituri already confirmed all the comments were addressed. I believe (2?) sequential branches have already been merged into this by him as well. So, we should avoid bloating this further and merge it. Additionally, I could not push to the main repository anymore (I assumed I would be able to do it but some permission issue prevented it) so I forked and created a !23 to add his suggestion for check_error_text() in his comment above. I suggest we merge that after we merge this to main.

DeepanshKhurana added 5 commits November 22, 2024 16:59

feat: add theming with config.yml

8089895

feat: add functions for coloring svg files feat: replace sass colors with css colors

chore: add onClick = "select" to both app list and job list tables

ac42cd1

chore: revert config color to Appsilon Blue

df18d7e

docs: update README with new FAQs about branding

2250cfa

feat: add handling for logo in config.yml theming

b33ef11

DeepanshKhurana added the enhancement New feature or request label Nov 22, 2024

DeepanshKhurana requested a review from jakubnowicki November 22, 2024 11:58

DeepanshKhurana self-assigned this Nov 22, 2024

jakubnowicki requested changes Feb 11, 2025

View reviewed changes

TymekAppsilon and others added 11 commits March 13, 2025 10:12

refactor: untangle main.R server

5543096

refactor: replace reactiveValues with plain reactives

d6eb5ff

refactor: update checks

c583c89

refactor: unnest renderUI

cc4ba3a

test: add unit tests for general_utils.R

1bd9d78

feat: move branding outside ui/server

d8cb1c0

chore: remove nolint

f2d48df

fix: add working default value + docstring improvement for generate_e…

d12bdcc

…mpty_state_ui

chore: move primary to the top in config.yml

9e1a293

chore: remove redundant default variable from generate_css_variables

9937baa

docs: add context for what the colors do in README.md

3a226a7

DeepanshKhurana requested a review from jakubnowicki April 13, 2025 11:58

TymekAppsilon and others added 9 commits April 23, 2025 09:07

Merge remote-tracking branch 'upstream/feat/add-config-theming' into …

8f88ddd

…refactor/simplify-state

chore: enable box.linters

937c239

feat: add config for llm mode in config.yml

70d1163

chore: update dependencies

7ff93c2

feat: add llm system prompt

56a3dba

fix: independent linting fixes

1b22202

feat: add llm utils using ellmer

ebe6d6b

feat: integrate llm functionality

54a06fa

feat: add llm styling

86b1d69

DeepanshKhurana added 3 commits April 23, 2025 13:19

chore: disable llm mode by default

3de3d6b

feat: add logic to validate llm providers

b634e10

chore: update R version + rebase

687c311

DeepanshKhurana requested a review from vituri May 6, 2025 08:00

vituri reviewed May 9, 2025

View reviewed changes

DeepanshKhurana mentioned this pull request Oct 3, 2025

Bug: No errors found triggers as an error #22

Open

		@@ -1,31 +1,17 @@
		# nolint start: box_func_import_count_linter

Conversation

DeepanshKhurana commented Nov 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Definition of Done

Uh oh!

jakubnowicki left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vituri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DeepanshKhurana commented Oct 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DeepanshKhurana commented Nov 22, 2024 •

edited

Loading