Remove basic tile override #206
Conversation
hvelarde
requested review from
rodfersou,
claytonc,
caduvieira,
idgserpro and
almeidajeff
There was a problem hiding this comment.
Fiz um teste, instalando uma versão 1.5.1 com um tile básico com uma imagem:
Depois, atualizei para o master (futuro 2.0a4). Na sequência, atualizei o brasil.gov.tiles para a branch desse PR, cleanup-basic-tile. Comentei a linha com problema do upgradeStep do nitf e alterei o tema para o novo no painel de controle.
Rodei os upgradeSteps dessa versão e a imagem no tile na capa simplesmente sumiu.
Tem de se verificar porque a imagem sumiu e analisar se será necessária algum passo via upgradeStep.
CHANGES.rst
Outdated
| @@ -4,6 +4,9 @@ Changelog | |||
| 2.0a1 (unreleased) | |||
| ^^^^^^^^^^^^^^^^^^ | |||
|
|
|||
| - Remove override do tile básico. | |||
There was a problem hiding this comment.
Complementar indicando que o recurso "Variação de Título" será removido com a remoção desse tile.
There was a problem hiding this comment.
eu prefiro esperar até termos a definição clara do que vai ser o IDG 2.0.
| required=False, | ||
| ) | ||
|
|
||
| form.no_omit('image_description') |
There was a problem hiding this comment.
O campo equivalente ao alt em collective.cover se chama alt_text. Será necessário um upgradeStep que copia do data manager do tile pegando do image_description e colocando em alt_text.
from plone.tiles.interfaces import ITileDataManager
ITileDataManager(app["Plone"]["home"].get_tile('b4a3278b-30d0-4328-bebe-ab375be6804e')).get()
> {'description': u'Foto no tamanho 664 pixels de largura com legenda em 90 caracteres para formar tr\xeas linhas', 'title': u'Imagem not\xedcia horizontal tamanho 664', 'image_description': u'Foto no tamanho 664 pixels de largura com legenda em 90 caracteres para formar tr\xeas linhas', 'image': None, 'subjects': True, 'css_class': None, 'date': True, 'variacao_titulo': u'Normal', 'uuid': '5a7e810751aa45049231f3b563c8d819'}Pode ser que o upgradeStep também será necessário para corrigir a imagem do tile que "some" quando é atualizado, ver #206 (review)
There was a problem hiding this comment.
concordo, mas não vou fazer agora os upgrade steps; não está incluído na demanda e não temos tempo nem recursos para isso; fica aqui documentado, mas gostaria criar em algum lugar um checklist de coisas que tem que ser feitas para não ter que procurar logo por todo canto.
| readonly=True, | ||
| ) | ||
|
|
||
| form.no_omit('variacao_titulo') |
There was a problem hiding this comment.
Não sei se é preciosismo, mas como esse campo será removido, deveríamos percorrer os tiles collective.cover.basic e, se existir essa chave no DataManager, remover.
There was a problem hiding this comment.
sinceramente acho completamente desnecessário: ao popular o tile de novo ou modificar o layout isso vai sumir de qualquer jeito.
| settings = registry.forInterface(ICoverSettings) # noqa | ||
| return settings.searchable_content_types | ||
|
|
||
| def getAlt(self): |
There was a problem hiding this comment.
Como esse comportamento de mostrar o título ou descrição do objeto se o alt text não for preenchido será removido (ver collective/collective.cover#778), é importante avisar isso no changelog, inclusive com a referência do motivo disso ser errado.
There was a problem hiding this comment.
isso não é assim, eu deixei o comportamento atual sem modificar: ele vai usar por padrão descrição ou título.
There was a problem hiding this comment.
Hoje, pelo getAlt, eu tento pegar o texto alternativo, se não tem, pego o título, se não tem, pego descrição. O tile Basic do cover tenta pegar o texto alternativo e, se não tem, simplesmente não mostra nada. Se o formato atual está errado (como apresentado por você em collective/collective.cover#778) ou não, você está mudando o comportamento/expectativa de uma funcionalidade e acho que o usuário deveria ser avisado disso.
Particularmente, acho que se agora o alt_text tem de ser preenchido para ser renderizado, até acho válida a discussão se ele não deveria ser obrigatório por questões de acessibilidade, mas isso é pra ser discutido em collective/collective.cover#778.
There was a problem hiding this comment.
acho que não estamos nos entendendo: o único motivo para ter essa condicional ai é para o atributo alt não sumir da tag caso o tile tenha sido cadastrado manualmente (já expliquei isso antes); usando drag&drop o campo se preenche com um valor padrão (descrição o título) como pode ser visto aqui:
por favor leiam atentamente o documento que mencionei acima; o atributo alt é uma das coisas mais difíceis de manter num site por as razões expostas lá. não basta, nem faz o menor sentido, simplesmente cadastrar por cadastrar; o exemplo usado ai é muito claro: o valor certo depende do contexto e deve ser feito por um editor competente sempre.
|
a funcionalidade que era adicionada no override de este tile (edição do atributo se ninguém mais tem comentários eu vou mesclar este pull request ficando pendente adicionar um upgrade step no futuro. |
|
É realmente trabalhoso remover as customizações: é necessário comparar o código, atributo por atributo, método por método de cada tile com o correspondente no collective.cover, tentar entender a motivação daquilo (se foi um hack e se não é mais necessário, se é uma funcionalidade específica de um grupo de clientes, etc), muitas vezes verificando histórico do commit, verificar se o que está no brasil.gov.tiles poderia ir para upstream em alguns casos (como nitf, coleção e etc), verificar se o formato de dados armazenado no IDataManager daquele tile é o mesmo (e se não for, desenvolver um upgradeStep que coloque no novo formato), e... testar bastante. Por isso não atuamos nessa força tarefa de remover as customizações até hoje. Em resumo: não existe uma lista do que precisa ficar/remover. É trabalho manual mesmo. Por isso o PR antigo de remoção do tile de lista em #173 ficou parado, pois com a mudança feita por collective/collective.cover#713 ele teria de ser todo revisado e naquele momento o cliente estabeleceu outras prioridades e definiu que ia ficar com tudo customizado como já estava, mas funcionando do jeito que ele estava acostumado. Vocês estão falando que vão se preocupar com a migração depois: isso pode dar problema/gerar retrabalho pois como disse, pode ser que uma customização aqui efetuada tenha de ser mantida, como foi no caso de https://github.com/plonegovbr/brasil.gov.tiles/blob/796a644875bb7ac01ae9cd4ea778bf7044e23c8f/src/brasil/gov/tiles/tiles/nitf.py, e vocês terão de readicionar o tile. Além disso, há uma perda de contexto mental ter de voltar em cada PR e fazer a comparação manual que mencionei. Não temos condição de fazer essa revisão minuciosa como mencionado no primeiro parágrafo, o recomendado é ela ser feita por quem está efetuando as alterações nas tiles, por estar mais familiarizado com a demanda e por demandar testes pontuais. Não há muito o que eu revisar nos PRs se não for haver migração, portanto serão dados apenas comentários relativos à remoção. Só um comentário final: não acho uma boa idéia um release 2.0 (ou seja, final release) oficial sem a rotina de migração de todos os tiles removidos efetuada. Lembre-se que o collective.cover é amplamente usado (temos clientes muito satisfeitos com a possibilidade de montar uma capa com essa versatilidade), basta dar uma passadinha em https://idg.receita.fazenda.gov.br/ para ver que "refazer capas na mão" simplesmente não é uma opção. |
|
@idgserpro eu entendo e comparto em parte seu raciocínio; faz um par de semanas eu pedi para vocês levantar todas essas questões na lista de correios para ter uma discussão mais ampla e continuo aguardando até agora. por outro lado, se as coisas tivessem sido feitas corretamente desde o inicio eu não estaria aqui agora removendo customizações e complexidades desnecessárias dos pacotes. nós não estamos propondo alterações sem sentido; estamos simplesmente limpando o código para fazer mais simples a manutenção. eu dividi ó trabalho de remoção das customizações em vários pull request diferentes precisamente para fazer mais simples o analise do que tem que ser feito; e vou continuar precisando de sua ajuda. por outro lado, ninguém está obrigado a atualizar sua plataforma; evoluir tem sempre custos associados. cada caso é um caso: nós optamos por refazer todas as capas dos sites que estamos atualizando e foi bem mais simples… é são sites importantes, posso garantir. |
hvelarde
force-pushed
the
cleanup-basic-tile
branch
3 times, most recently
from
65ea4de to
4108965
Compare
|
@idgserpro posso confirmar o problema da perda da imagem mas isso é ocasionado por um bug no branch 1.x (diferente do collective.cover, o campo
o pull request está pronto, eu sugiro abrir uma nova issue no branch 1.x, corrigir lá, e mesclar isso aqui para poder continuar os trabalhos. |
|
@idgserpro algum feedback sobre isso aqui? |
hvelarde
force-pushed
the
cleanup-basic-tile
branch
from
4108965 to
8435f1e
Compare
supersedes: #202