| Project: | Import HTML |
| Version: | 6.x-1.x-dev |
| Component: | User interface |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
Issue Summary
This is a great module; I thought it would be a daunting task to get everything working right and understand the import... but with the help of the documentation, I was lead straight where I needed to be.
Despite the user interface. Because that is confusing. Two main points:
* there's two menu entries where you can configure profiles: the 'profiles' and 'settings' sections. That makes it confusing for people trying to find their way. Especially when the 'profile summary' contains a links to the 'settings' (not 'profiles') section.
* The first 'import site' page contains on one screen:
- a select box that is always set to 'default' at first
- a number of overrideable settings (which are therefore always taken from the 'default' profile,
- a 'next' button.
i.e. if you configured those settings in a non-default profile.... that's for nothing. You'll never use them. This is not 'confusing', it's a plain UI error.
How to solve it? I guess that depends on taste. (But the 'profile selection' needs to be moved away from -before- the mentioned page!)
I have a solution that I'd like to run by you for feedback:
- moved the 'links to sections' on the main page to standard Drupal 'tabs'
- the main page now contains a table of profiles, with link to 'edit profile' and 'import HTML site' -- essentially merging 'admin/build/import_html', 'admin/build/import_html/import_site' and 'admin/build/import_html/profiles' into the same page
- A profile now has to be specified in order to start an import process, like 'admin/build/import_html/import_site/'
- removed the 'profile summary' from places where IMO they weren't needed. It's visible in one place only instead of 3 now.
Thoughts, anyone?
The patch is not ready for inclusion yet; it just gives an impression. It works, but the code is still ugly and needs to be cleaned up.
[ dman: speaking of that: what's the plan with import_html_current_profile_id()? The 'import_html_current_profile' variable is never set, right?
I'll need to overhaul something there, together with import_html_variable(), because currently that triggers the message, if a profile has the 'debug level' set. And maybe in other circumstances. ]
| Attachment | Size |
|---|---|
| ih_ui.diff | 15.33 KB |
Comments
#1
The profile ID thing is (what looks like unnecessary) clutter to the one-off user.
It's a low-level architecture thing I built in to support multiple profiles at once, as prep for a bigger realtime 'mirror' system
So I can have sections of a site that are filled in via an automated process. I've got stuff in dev that will support total remote site cloning, and some presets for different types of source material.
I've got another module which leverages import_html internals, and needed it to be able to pick the settings up from individual configurations. In d5 all the settings were semi-global.
I think we can probably add an 'advanced' flag to just hide that chunk of the UI from normal users entirely. Just assume 'default'. It is in the way right now, I agree.
Moving some of the menu items into tabs is probably a good idea. Never stopped to think about that at the time.
Making the default main page a little more coherant overview is a good thing. The current look is from when there only used to be one thing to do.
I abstracted the 'profile summary' into its own re-usable block and re-used it in the UI again and again to really push the status information in your face. Because there's 20 things that might not be right, I thought it would be helpful to be verbose. In iterative testing I always wanted to know exactly what was about to happen this time.
Thanks for the input, I'll ponder
#2
> I abstracted the 'profile summary' into its own re-usable block and re-used it in the UI again and again to really push the status information in your face. Because there's 20 things that might not be right, I thought it would be helpful to be verbose.
Agreed.
FYI:
* I still left it on the 'import HTML' page, but moved it down a little, to just above the 'overrideable settings'
* Removed it from the 'settings' section, because IMHO 'settings' and 'profiles' should really be split, to decrease the chance of 'going in circles / getting lost'
* Removed it from the 'edit profile' section, because IMHO you don't need an display of settings, if you have (filled) edit controls for the same settings on the same page.
If I removed more, that was unintentional.
#3
Further thoughts.
I've gone through your patch line-by-line.
My dev code has diverged a bit, but even running on a clean copy of recent dated versions I was unable to get a clean patch to apply. Some of the repetative code in the menu func confused the diff too much. But I see it's not up for a real release anyway, so I'll just cherry-pick the bits that make sense right now.
- we can move the profiles into sub-tabs, like Drupal theme management does. If you never have more than one, you'll only see the 'default' settings. 'new' profiles will be an afterthought, not a primary choice.
I don't care too much for a few aesthetic changes - like uncollapsing the htmltidy info. It's collapsed when it's got nothing to say, and adding a confirm step to the 'delete profile' is hardly worth it.
If you've deliberately uncollapsed all the fieldsets, then yeah, you don't particularly need the summary at the top of the page. But with them collapsed and the page simpler, the summary is a lot more relevant.
It could be that new users would prefer to see all options open, and I prefer them closed because I know what the options are. I can bang through without scrolling.
Maybe I'll fix them open until the profile has been saved for the first time...
The wording clarifications make sense.
I'll see what I can do to the front UI page. I can't see exactly what you were aiming for, but you've convinced me it needs some love.
#4
Woot.
OK, I got inspired and implemented about half the things in that suggested patch.
Then added a dozen more smooth-outs.
If you don't use multiple profiles, the choice doesn't show up.
I left out the confime delete button, but sorta addressed most of the other changes I could see and agree with in the patch.
This new dev revision is quite a huge one. I touched a lot of little things, and specifically the places you were touching so there's not much chance you can merge.
Hope I haven't broken any actual process in the process, but I'v been building unit tests (!!) into the thing last week. Once I get some test case coverage I'll know what I've (not) broken and roll a release!
I've committed to CVS already, so calling this suggestion fixed. Anything else you can suggest (some test cases would be good) would be welcome.
#5
I am using the current dev release, and the changes seem to be a good direction for me.
One thing, the font size in the import log is impossibly small, I had to kick up the text size in the browser. Perhaps this is an artifact of my theme, I am using CTI Flex, a Zen subtheme.
#6
It's good stuff. And now that the whole 'settings' (for htmltidy binary) is apparently gone, the overlap between 'settings' and 'profiles' is not there anymore, which is good.
One thing I want is included in this very small patch. In a new profile screen, the name should be empty, not 'new'. Because now, if you mistakenly leave the 'new' filled and you press save, you get the message "Not saving details called "new"" -- and all your carefully selected settings are gone / reset to default.
(I'm sorry for the confusing layout of the patch. I only removed the '#default_value' line. Plus I just had to reshuffle a few lines, sorry, it's a compulsion :p)
=====================
Further, I like or agree with most of the layouts.
But still think the 'Import HTML' screen itself is just 'bad UI'. Like I said in the 'second point' of my first message, but in different words:
- you will see the profile settings from the 'default' profile printed on the 'step 1/3' page, even if you select another profile. That's a bit confusing.
- You will always have the "Overrideable Profile Settings" from the default profile selected. which is just 'bad UI'.
(My tests actually seemed to indicate that these "Overrideable Profile Settings" on this screen were ignored, when I selected a non-default profile from the select box. But I might be wrong.)
One way you can prevent this, is to ditch the select box on the top of this screen, and replace it by tabs, just like in the 'Manage settings' section.
(I'm not providing a patch for this at the moment, because you may not agree with me.)
#7
Clearing out the 'new' name is probably a good idea, yeah.
IIRC, in development once, submitting a profile with a blank name accidentally saved it with a name of "" which proved a bitch to find and remove. A little more validation should prevent that from happening again. The use of FAPI probably needs a little more work if it was wiping previous settings on validation. Shouldn't happen, but I can imagine why it is.
If that's happening, it's a bug. In this recent code I tried to make the previous choice 'sticky' so it should remember what you last did. That wasn't happening at all before. I'll have a look.
That's part of the reason I repeated the info about "what you are about to do" on the next screen. Without AJAX, you wouldn't see that a selection on screen 1 had any effect.
Perhaps the wizard should be a 4 step process - starting with choosing the profile before even seeing the next settings, but I'm trying to avoid that, and just dropping you in to the most straightforward normal case.
The overrides are supposed to be temporary tweaks, mostly put there to support choosing taxonomy terms on a per-round basis - so you can repeatedly import different sections with different selected tags, paths.
There is a chance that that idea is indeed broken now that the 'batch' mode has been introduced. That would be a bug and may be tricky to work around.
Originally when processing happened all-in-one the form submission info was available. In batch mode, less so, because the queue saves only filename+profilename (not full profile settings, due to frustrating size limitations in batch API). Saving filename+profilename+tweaks would require new refactoring.
It may be that this advanced option is too much, but it was handy during complex jobs and test runs.
I'll see if I can put this 'new' name patch in at least... Certainly a nice little UI polishing, something I'd miss because I knew what I was doing...
#8
I disabled and uninstalled (using the admin UI) and installed and enabled using the dev release stamped Feb 02 2010.
Created a new profile (had a space in the name, in case that's relevant) and it did create one, with the message, first bullet:
import_html_variable initing profile from nowhere - should this ever happen?(in case this is debug code, I thought I would let you know.)
The second bullet in the confirmation message was
XYZ ABC Profile Updatedwhen what it actually did was create it, not update it. Just terminology, I guess.
Finally, after saving a new profile, it seems to still be the create profile page offering to create another profile called new, which is not where I thought I'd go. I'd rather go back to the admin\build\import_html level so I can so something with the module using my new profile.
#9
That is sorta debug code, yes. Probably doesn't need to be user-facing any more. It was to warn if I was accidentally initiating a blank profile at any stage, because that would be a hole in the logic.
Update/create, yeah, but it's not worth the bother flagging a simple setting as is_new. These are just extended variables getting set here.
Yeah, I noticed the save didn't take you anywhere. Wasn't sure how to redirect forms correctly a few years ago. Should be fixed up a little
#10
Not anymore. The field is 'mandatory', so if you try to save, the FAPI will abort and create a nice red border around your Profile ID textbox. The included patch 'just works'. Although it does not remove the now-unneccessary "not saving 'new' profile" message from the code.
No. If you think so, then I should rephrase.
By the way, while testing I hadn't spotted the fact that you made the previous choice 'sticky'. That's cool. And it actually fixes the gripes I have with the module in practice. So I will stop whining after this message. Promise ;-)
Here's me rephrasing:
In the 'step 1 of 3 screen' you have:
-A. a section detailing your settings, in text. Let's call this "Profile X" (I called it 'default' and shouldn't have)
-B, a secton with some extended settings, part of which were defined in Profile X.
-C. an ability to select Profile Y or Z instead, up top -- which ofcourse does not update the settings mentioned at 'B' (or the message at 'A', to reflect the actual situation).
-D. a 'next' button.
The combination of C and A is confusing unless you know the intricacies of the module.
The combination of C and B is what I call 'bad UI'.
You say "the overrides are supposed to be temporary tweaks" but I happen to disagree. I have defined a non-default "User to create nodes as" in my profile. I like having it. But that profile setting will be ignored, because there's also a "User to create nodes as" on the 'step 1' screen, which I now have to recheck to make sure it's actually right.
(Yes, the 'sticky' choice helps a lot, that's true. IF you know EXACTLY how the module behaves already, THEN you will not be confused by what's prefilled for those values in the 'Step 1' screen. And THEN you will know that the "User to create nodes as" profile setting actually has some use. Once you know how.)
I don't see that repeated info. I just see a bunch of checkboxes to select files, and a "Import HTML" button. Oh yeah, and a one-line notification about which profile is used.
I'm apparently really missing something with the AJAX stuff. Is it my browser (IceWeasel on funny Debian install)? I don't recall having other problems with it...
IMHO, conceptually a 4 step process would be the right way to do things, yes. Creates less confusion for users who don't "get" the overlapping things on page 1. And you can achieve the same effect by keeping a 3 step process with different "entry pages".... replace the 'profile select box' by a range of tabs, just like you've done in the 'Manage Settings' section.
---
OK. I'll stop whining now. ;-)
Since the 'current profile ID' (which starts an import) is retrieved from the database... I know how to tell my users to import stuff using one of two very similar profiles, without confusing them...
#11
ah, um.
So remove the selector at C altogether? I found it reduced the steps, but yeah I did have to know what was going to happen.
But I guess that is what's leaving it open to confusion if you apply the 'temporary' settings - inheirited from the previously sticky profile and choose a different profile in the selector. the previous 'temporary' settings will now actually override your profile choice. Which is certain to be wrong.
Right, ditch the selector and only use tabs.
You don't now. We removed it earlier (I think).
There is no AJAX happening. That was never put in, I'm just saying that that would be a way to update the text summary (and indeed the overrides) when you switch the selector. It's not happening because it's complicated.
At this rate it'll almost come full circle and have the settings page show everything with just a "Proceed using these settings" button at the bottom.
Having a set of tabs for Step 1 (and step 1b, 1c) that allow you to choose profiles and proceed
and a set of tabs for profile a,b,c settings that's almost but not quite the same is starting to look odd.