Skip to content

[4.0] Modify display and tip for Edit Article in Frontend#30588

Merged
laoneo merged 6 commits intojoomla:4.0-devfrom
infograf768:4.0_edit_article_cassiopea
Sep 9, 2020
Merged

[4.0] Modify display and tip for Edit Article in Frontend#30588
laoneo merged 6 commits intojoomla:4.0-devfrom
infograf768:4.0_edit_article_cassiopea

Conversation

@infograf768
Copy link
Member

@infograf768 infograf768 commented Sep 7, 2020

Summary of Changes

Since Email to a friend and Print article have been taken off from J4, we do not need the anymore the Cog and dropdown to choose among actions. We now need only the Edit Article when authorised to do so.
Use of role="tooltip"] instead of bootstrap tips.
Cassiopea [role="tooltip"] is modified for the look. Placement of tip specific to the article edit id.

Should also be OK for a11y. TO CHECK please as I'm not a specialist.

Testing Instructions

3 situations:

  1. Edit Article when article published // simplified tip
  2. Edit Article when article is unpublished // simplified tip
  3. Article is checked out by another user

In each case, hover the icon to see the tip.

patch, run NPM

Actual result BEFORE applying this Pull Request

Screen Shot 2020-09-07 at 18 35 48

Screen Shot 2020-09-07 at 18 37 57

Screen Shot 2020-09-07 at 18 37 23

Expected result AFTER applying this Pull Request

Screen Shot 2020-09-07 at 18 15 40

Screen Shot 2020-09-07 at 18 14 48

Screen Shot 2020-09-07 at 18 13 31

Note

If this is OK, will do for Contact

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.0-dev labels Sep 7, 2020
@infograf768 infograf768 added the a11y Accessibility label Sep 7, 2020
@brianteeman
Copy link
Contributor

I have tested this item 🔴 unsuccessfully on 741f9ca


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30588.

@brianteeman
Copy link
Contributor

The accessibility changes you made are completely wrong

  1. never add role=button to a link
  2. incorrect usage of aria-live (and not needed)

@infograf768
Copy link
Member Author

infograf768 commented Sep 8, 2020

The accessibility changes you made are completely wrong
never add role=button to a link
incorrect usage of aria-live (and not needed)

It was useless to set unsuccessful as I noted in the description TO CHECK please as I'm not a specialist.

Thank you for your comment. Getting rid of these 2 aspects. Is there anything else?

@brianteeman
Copy link
Contributor

I cannot replicate your screenshots - looks like you might be missing a class on the "tooltip" div

image

image

@brianteeman
Copy link
Contributor

Also because the link is called edit and the tooltip begins with edit it is announced by a screenreader as
Edit Edit ....

@infograf768
Copy link
Member Author

infograf768 commented Sep 8, 2020

NPM ci is necessary.

Class is not missing, role tooltip is a class de facto

// set up hidden tooltip
[role="tooltip"]:not(.show) {
  z-index: $zindex-tooltip;
  display: none;
  max-width: 100%;
  padding: .5em;
  margin: .5em;
  color: $black;
  background: $white-offset;
  border: 1px solid $gray-600;
  border-radius: $border-radius;
  box-shadow: 0 0 .5rem rgba(0,0,0,.8);

  &[id^=editarticle-] {
    margin-inline-start: -10em;
  }

  [dir=ltr] & {
    text-align: left;
  }

  [dir=rtl] & {
    text-align: right;
  }
}

but it looks like I can't apply anymore this .diff in eclipse for the 3 layouts files
Checking that aspect now here.

Can you explain

Also because the link is called edit and the tooltip begins with edit it is announced by a screenreader as

Not sure what you mean by the link is called edit. Can you explain? And what it should be?

@infograf768
Copy link
Member Author

I see now why this PR does ot apply anymore here..
#30543 was merged

@infograf768
Copy link
Member Author

The git apply issue solved itself here.. :)

@infograf768
Copy link
Member Author

Also because the link is called edit and the tooltip begins with edit it is announced by a screenreader as Edit Edit ....

Got it now. I don't think it is a big deal.

@brianteeman
Copy link
Contributor

That would be a decision by the accessibility team

@infograf768
Copy link
Member Author

Stefan made suggestions. Will modify stuff shortly.

@infograf768
Copy link
Member Author

Modified with various improvements. aria-hidden true. Simplified tooltips.
Now we get:

Screen Shot 2020-09-08 at 17 28 10

Screen Shot 2020-09-08 at 17 24 42

Screen Shot 2020-09-08 at 17 26 56

@hans2103
Copy link
Contributor

hans2103 commented Sep 8, 2020

I have tested this item ✅ successfully on f9fd8ed

I see the simplified tip in all three situations


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30588.

@brianteeman
Copy link
Contributor

Don't know why but the role=tooltip css is present in my unminified css file but NOR in the minified one

@chmst
Copy link
Contributor

chmst commented Sep 8, 2020

@brianteeman did you use the patchtester? I have the same effect, but think that this not related to the PR. The PR looks great I will test as soon as required the changes are comitted.

@brianteeman
Copy link
Contributor

@chmst I did but I cant see how that would make a difference. If the code is present in the non-minified css then the patch has worked

@brianteeman
Copy link
Contributor

found the problem - its not related to patchtester - creating a seperate issue for it

@brianteeman
Copy link
Contributor

I have tested this item ✅ successfully on f9fd8ed

Marking as successful as the minification issue is unrelated to the actual pr


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30588.

@infograf768
Copy link
Member Author

Don't know why but the role=tooltip css is present in my unminified css file but NOR in the minified one

Screen Shot 2020-09-09 at 07 39 21

@infograf768
Copy link
Member Author

RTC as last changes were small cs corrections.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30588.

@infograf768 infograf768 removed the a11y Accessibility label Sep 9, 2020
@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Sep 9, 2020
@infograf768 infograf768 added this to the Joomla 4.0 milestone Sep 9, 2020
@laoneo laoneo merged commit 9a42399 into joomla:4.0-dev Sep 9, 2020
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Sep 9, 2020
@laoneo
Copy link
Member

laoneo commented Sep 9, 2020

Thanks!

@wilsonge do we need to document the change that the helper function got removed in the upgrade guide?

@infograf768 infograf768 deleted the 4.0_edit_article_cassiopea branch September 9, 2020 07:55
@infograf768
Copy link
Member Author

Will now make similar changes for the Edit Contact.

@wilsonge
Copy link
Contributor

wilsonge commented Sep 9, 2020

@laoneo I guess it can't hurt to document it although I suspect it's a touch overkill

sakiss pushed a commit to sakiss/joomla-cms that referenced this pull request Oct 16, 2020
* [4.0] Modify display and tip for EDit Article in Frontend

* take off commented line

* correcting wrong a11y parts

* various improvements

* cs

* cs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Language Change This is for Translators NPM Resource Changed This Pull Request can't be tested by Patchtester

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

Comments