Overhaul CSS for maintainability and customization #3862
Conversation
|
I have picked some of your commits and pushed them onto master. |
|
@timoschilling awesome thanks! I think I did the rebase right. Let me know if there are any issues. I'll be continuing to work on this tonight. |
|
@timoschilling do you know anything about Popover vs DropdownMenu? I'm so confused as it seems we only use the second one. I ask because we have what looks like similar CSS for both. Do we need to support both? |
|
Dropdown: |
|
@timoschilling awesome thanks! Looking into it. I'm adding a new label to the en.yml locale. The label is "commented on" to appear in admin comments. The comment layout has changed to be more in sync with common layouts like here where author/date appear first and then comment body. Should I translate the text for other languages for when merging? Happen after? Have to ping authors for help? |
No that's not necessary, en is the reference locale, every other locale is maintained by the community. |
What are your problems? Maybe I can help you. |
|
@timoschilling just a lot of issues with styling Formtastic. It made a poor choice with its HTML structure. It improved a bit with adding certain classes. Struggling with how we style fieldset and legend to look like panel and title. It gets hard to use in other contexts since you constantly have to turn off styles. Ideally would be best to remove it. Maybe we can update the "form" active_admin DSL to output the form in a panel wrapper but don't think that will be easy since the title is applied on f.inputs. I think its in a good place for now.
|
|
@timoschilling I'm having a hard time figuring out how to combine 2 Arbre elements. I'm editing table_for so that a table header includes a span for the sortable arrow (will be styled using CSS arrow, no image). When I try this I just see the span tag, no link and span for a sortable column: def build_table_header(col)
# ...
th class: classes do
link_to(col.pretty_title, params: params, order: "#{sort_key}_#{order_for_sort_key(sort_key)}") + ' ' + span(class: 'sortable-arrow')
end
# ...
endWhat should I do to get the desired output of |
|
Great news. Been able to cut out a lot of the has_many form styles since now we have better defaults in place. The default layout is looking good. Screenshot below.
|
|
Can you replace this arrows too? |
|
No worries, my intention is to clean up status_tag further. I wasn't sure yet what to do with ok/completed state. I didn't know of #3677, this is exactly what I needed, thanks! I'll go through it and make all changes I can. If I have trouble I'll let you know. On the has_many form arrows, you're right, those are using Iconic SVGs. I'll work on a replacement for that. |
|
Scopes are a bit incorrectly displayed. In old design: In a new design: |
|
Everything else so far seems OK in one of the big projects that uses AA. |
|
@javierjulio looks like you make a great job! But your PR is with ~90 commits and if I take a look on your todo list I think you hit the 150-200, on of the biggest PR's ever. Maybe you can split out some commits in it's own PR's like:
Do this only for commits which are working already on the master. |
What do you want to change? And why it's necessary to change the markup? |
|
Thanks @dmitry for trying it out. Can you please make sure to have my latest branch changes in your ActiveAdmin instance? I have these components styled rather differently now so I'm pretty sure not all my changes are in place. For reference below are some screenshots of what the components by default will look like without any additional changes.
|
|
@timoschilling thanks! 😄 This was always meant to be a major task. I didn’t know the full changes since I wasn’t familiar with all the components in ActiveAdmin so naturally the todo list grew. The problem is many of these commits are dependent on one another. I can’t just break them apart into separate PR’s because several of the changes have side effects (for example, switching to normalize.css is a good one) that I handle over time. Also the same files have changes across commits so with separate PR's I'll just run into more merge conflicts. It would be really time consuming to break these apart and make sure to include all other related changes but then I will miss necessary changes. 😩 Unless mistaken I don’t think its a concern since I would have to rebase master and squash my commits for you to merge right? That means I’ll handle any conflicts and on your end it should be easy. I’m done with the major changes. The few remaining items in my todo list aren’t crucial at this moment. Now what’s left is to configure important styles in variables to allow easy customization. Do you want me to start on that in this branch or get this rebased and merged in and then in a separate PR do that? My guess is that since users run off master probably best to get the variables in first? Let me know what you think. |
|
I replaced the drag arrows for the has_many forms with a simple grip looking action. @timoschilling this is just a default but we can modify if necessary. I looked at what is used to indicate dragging and I think this was the best choice. The other was to use horizontal lines but can't get help the feeling that means hamburger style nav menu.
|
|
@javierjulio pointed out. I've removed
One issue found so far. I have a header column in one of my models, and it renders with .header css:
Great work so far! 👍 PS. That's why I like BEM in such cases. |
I know it's not easy and some changes are depending on each other, but if you find some, it will be help.
Sometimes I like that practice while some just refactor or fix his code 3 times with commits like:
In your case we can maybe left all commits to see what has changed why.
That should be done in this PR. We write this PR in the braking changes and link to the commit with the new variable list.
Lets give them a try. |
|
Really nice work @javierjulio, was going to tackle this myself before I saw your PR. What would be the best way for me to help get this merged? |
|
I wonder though, what was the logic behind getting rid of the blank state border? The text looks a little lonely to me now by itself 😢 |
|
@mattvague thanks! 😄 Would be great to have some help on this I just need to jot down tasks in more detail in the todo list above. I'll update you later tonight. In the meantime it would be a huge help to use this branch in an existing app to see what the admin looks like to find any visual quirks. There are a ton of components/features of ActiveAdmin I didn't know about or didn't use. I set them all up in a test app so I'm pretty confident I have all cases handled but you never know. I removed the border because I found it tacky and taking away from the content. The lighter text color I found sufficient enough and it looks clean/smooth with the right amount of spacing. Less distraction. |
|
@dmitry awesome! Glad those components are good. Ha sorry about that. Yes, I renamed the header selector. I really don't like using id based selectors. Definitely going back to rename that as I know its very generic. My plan has been to prefix with "app-" so you'll now have app-header and app-footer for example. @timoschilling if I find something small I will do that for you but don't be surprised if you get nothing haha I just can't imagine doing that now because its not just a matter of taking a commit from here but then making sure it still works fine in the context of the CSS before I started (they are very different now, very streamlined, assumes various defaults). But in the meantime I will be submitting a PR for removing popover code so expect that soon. Sounds good to me on keeping the variable changes in this PR and merging in all commits to preserve history. |
Sounds good.
Just tried it on a few of my own apps and it looks good, couldn't find any issues
Hah alright, fair enough |
Update title bar component with new class names to make it easier to override if necessary.
This makes it much easier to apply button styles across the admin and allows users to specify their own themes too. For example, user provided action items now require that you set button class and theme class rather than just styling them all the same. You could apply different classes to each. Further this cuts down on our CSS and promotes better reusability for users by simply using class. Add button and button-primary class to Devise views Remove unnecessary position handling from dropdown JS Update modal dialog JS with explicit button classes
Now works pretty much the same as a Panel component. This way we can use `blank_slate` with a block and specify whatever elements we like rather than passing a content string (although that is still supported). Now our dashboard template is much cleaner as we use the built in component rather than hard code CSS and styles. Apply button style to blank slate action on index. Display text in a paragraph so button is below it but spaced out using our primary button style. Further configure styles in variables.
The previous color scheme didn’t feel very in sync so I’ve used the Flat UI color theme here across the board. I’ve cleaned up variables so we only have now the new scheme going forward. Variables that were removed or renamed are now in a commented section at the top. I’ve configured most of the Flat UI theme colors we use in variables with the same matching name from the theme. Some are only used once but figured it’d be good to just configure them by name so its easier to find which ones we are using if someone else is curious or wants to contribute.
We already have existing components to make it stand out so lets reuse them. Also updated the “recommendation” localized message to just link to browsehappy.com instead as that does the job.
Now setting a max-width on form rows so they don’t extend the full width. Provide variables for setting form label and input width while DRYing up styles around that. Fixed styles on base errors (these are shown above the form, not inline).
Lots of padding on various components make its difficult when used in other areas since then you start doing a lot of overrides (e.g. login form). Instead we’ll add the padding consistently on the fieldset (container) since formtastic always outputs one.
No need to override <ol> element as we now have consistent padding on fieldset. No need for link style since we include the secondary authentication link as a secondary action in the form (sticks with formtastic’s style by default). This should be good enough for a default and then others can customize as they see fit. Replace #login with a .devise-login-container class.
While we changed the settings for QualifyingElement in the scss lint config this is better since its descriptive about what type of element we are targeting. This would be for a fieldset containing dropdowns for example.
I kept some selectors as they were but several we can move around since they have specific class names. Being familiar enough with the code base its a safe change.
Reuse the caret class component and specify up or down based on sort order. Place the caret next to the text so its not off to the right edge of the column. Only show the caret if a sort is actually applied.
No idea how these are clashing since one is lowercase but there is some crazy dynamic stuff going on here. From what I can tell there is no way to access Rails helpers via a unique scope, its just referenced globally. The issue I ran into is on the generated test app, we have a Tag model and an admin page for it. When viewing the /tags/new page it raises an error on this line but it doesn’t do it for another model at the new page. It has to be a name clash. Further I have no idea how to get arbre to work nicely with Rails helpers and it can be a massive PITA. Best and simplest way I could resolve this issue.
Normalize went through some changes on v6 and v7 and since v7 is the latest it sets a default font family and line height but we don’t want that. While it is normalizing styles, we’d prefer that form controls inherit so they pick up the custom font we set as a default or a user sets on their end.
This was broken in commit 86e4a7d as it removed the class name. Its necessary to keep the scopes inline with the batch actions button.
As recommended when running `bin/yarn build`.
65deffc to
dd834bf
Compare
This doesn’t add any behavior, only modifies appearance for an extreme case. No actual functionality is broken because of this and the presentation part is dealt in activeadmin#3862 as CSS where it should be.
|
We are nearing a v4 beta release of ActiveAdmin that uses TailwindCSS through cssbundling-rails. We'll use Flowbite JS, @rails/ujs and our own vanilla JS all through importmap-rails. The release will have mobile web and dark mode support. Many elements (layout, html head, site/page header, main nav, user menu, flash messages, footer, etc.) have been extracted to partials that you can now customize, removing the need for many of our configs. With partials, you should now be able to use a different asset management gem, e.g. shakapacker, vite_rails, etc. without depending on the library to provide it. Many other improvements and changes are included. It also fixes that app-helpers-reload bug as well. 🙏🏻 Thanks everyone! ❤️ |
I've seen mention about CSS overhauls but nothing seems active, if anything years old. I'm streamlining the CSS to make it easier to maintain, improve and customize. This should make it much easier to move to a framework like Bootstrap or Foundation as well.
Since this was a long lived branch, several changes that were made here have been pulled out and merged into master to make this smaller. We've cut the number of commits to almost half!
How can you help?
Changelog
button button-defaultorbutton button-primary.button button-defaultfor a default button, orbutton button-primaryfor the primary button style.box-sizing: border-boxnow applied globally to all elements.nested_menu_arrow.gifwith css #3612