-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Add readonly ApplicationConfig to /internal/config #5662
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @citizen428, great job!
A couple of UI tweaks I think we should apply:
-
the sections should be opened by default, great idea to add toggles by the way!
-
there should be empty space between the first section and the second:
this way the button kinda disappears in the flow
@rhymes Thanks for helping the aesthetically challenged 😉 The sections are now expanded by default and I added some spacing between them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. don't forget to remove WIP
unless you're still working on it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense to me. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a typo :) Looks great otherwise!
</div> | ||
<% end %> | ||
</div> | ||
<//div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think this should be </div>
Although it renders fine anyway 🙃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking again this whole div
seems to have snuck in, probably some autocomplete mess up.
BTW, I won't get offended if you fix typos etc. on "my" branches yourself, especially if it means we can get things merged.
531b75c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
What type of PR is this? (check all applicable)
Description
This adds the values from
ApplicationConfig
to/internal/configs
.Note: there are quite a few values, 73 in total. So I opted for a slightly less sophisticated approach, basically just using some Ruby methods to iterate over the
ENVied
config. Since these values are read-only, I thought we could get away with this, since you'd have to editEnvfile
in order to change them, which includes more comments. This was done both to save space on the page, as well as save time in developing this.Related Tickets & Documents
#5384
Mobile & Desktop Screenshots/Recordings (if there are UI changes)
Since the page got quite long, I made sections that can be toggled:
The
ApplicationConfig
section looks like this:Added to documentation?